-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[Clang] Fix dependence handling of nttp for variable templates #69075
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
Conversation
5f3557e
to
7a5eef0
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
7a5eef0
to
e7c7969
Compare
@erichkeane @cor3ntin could you review this PR, please? I’m not a collaborator, so I could not add reviewers by myself. |
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 needs a release note, else I think it is OK.
443851f
to
99f5d7e
Compare
99f5d7e
to
0ee8b54
Compare
@erichkeane could you help me merge this PR please? I could not merge it by myself because I don't have write access to the repo |
c357787
to
dee8757
Compare
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.
A couple of nitpicks
TemplateKWLoc, NameInfo, TemplateArgs, Begin, End, false, | ||
false, false), | ||
TemplateKWLoc, NameInfo, TemplateArgs, Begin, End, | ||
KnownDependent, false, 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.
/*KnownInstantiationDependent=*/false, /*KnownContainsUnexpandedParameterPack=*/false
to be consistent with bugprone-argument-comment
@@ -1299,8 +1299,9 @@ static bool checkTupleLikeDecomposition(Sema &S, | |||
// in the associated namespaces. | |||
Expr *Get = UnresolvedLookupExpr::Create( | |||
S.Context, nullptr, NestedNameSpecifierLoc(), SourceLocation(), | |||
DeclarationNameInfo(GetDN, Loc), /*RequiresADL*/true, &Args, | |||
UnresolvedSetIterator(), UnresolvedSetIterator()); | |||
DeclarationNameInfo(GetDN, Loc), /*RequiresADL*/ true, &Args, |
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.
/*RequiresADL=*/true
The dependence of a template argument is not only determined by the argument itself, but also by the type of the template parameter:
For example:
The constant expression
Equal
is not dependent, but because the type of the template parameter is a reference type andEqual
is a member of the current instantiation, the template argument ofJoinStringViews<Equal>
is actually dependent, which makesJoinStringViews<Equal>
dependent.When a template-id of a variable template is dependent,
CheckVarTemplateId
will return anUnresolvedLookupExpr
, butUnresolvedLookupExpr
calculates dependence by template arguments only (theConstantExpr
Equal
here), which is not dependent. This causes type deduction to think thatJoinStringViews<Equal>
isOverloadTy
and treat it as a function template, which is clearly wrong.This PR adds a
KnownDependent
parameter to the constructor ofUnresolvedLookupExpr
. After canonicalization, ifCanonicalConverted
contains any dependent argument,KnownDependent
is set totrue
. This fixes the dependence calculation ofUnresolvedLookupExpr
for dependent variable templates.Fixes #65153 .