-
Notifications
You must be signed in to change notification settings - Fork 22
Fix alias sfinae #1058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Fix alias sfinae #1058
Conversation
An automated preview of the documentation is available at https://1058.mrdocs.prtest2.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2025-10-09 08:56:30 UTC |
if (Other.getKind() != TemplateArgument::Type) | ||
if (Arg.getKind() != Other.getKind()) | ||
{ | ||
return false; | ||
} | ||
return context_.hasSameType(Other.getAsType(), Arg.getAsType()); | ||
if (Arg.getKind() == TemplateArgument::Type) | ||
{ | ||
return context_.hasSameType(Other.getAsType(), Arg.getAsType()); | ||
} | ||
if (Arg.getKind() == TemplateArgument::Expression) | ||
{ | ||
return context_.hasSameExpr(Other.getAsExpr(), Arg.getAsExpr()); | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't handle the other template argument kinds. I think you want isSameTemplateArgument
.
But comparing template arguments like that only makes sense if the parameters are equivalent, as otherwise they'll have different conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only attempts to handle the boolean case. Given
template <bool C, typename T>
using enable_if_t = typename std::enable_if<C, T>::type;
the goal here is to match the C
that is an argument to std::enable_if
to the C
that is a parameter of enable_if_t
.
The pre-existing case attempts to do something similar for T
. Nothing else (no heroics).
Would isSameTemplateArgument
be a better choice here? Or is there a simpler alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSameTemplateArgument
does what this code does, and more, since it handles the other template argument kinds as well, such as template template arguments, the other kinds of converted constant template arguments, packs, and such.
What I mean is, if we are comparing template arguments to equivalent template parameters, then it makes sense to compare them like this.
If they are otherwise not equivalent, for example you are comparing a template argument for a constant template parameter of type bool
, to another one of type int
, then this won't work as expected.
Oh! That’s so nice! @mizvekov see? We pushed for change :) |
fixes #1057
When SFINAE detection involves an allias, the implementation confuses the controlling params of the underlying type with those of the alias, which only works when both have equivalent template parameters. This PR makes an attempt to handle the most basic cases by mapping between the two template parameter sets, and avoids an out-of-bounds access in the other cases.