mizvekov added a reviewer: mizvekov.
mizvekov added inline comments.
================
Comment at: clang/lib/AST/TemplateBase.cpp:373-374
case TemplateExpansion:
- return TemplateArg.Name == Other.TemplateArg.Name &&
+ return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+ Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
----------------
I believe this change is not correct, as here we want to compare these two
template arguments to see if they are identical (structural equality), not just
that they refer to the same thing.
================
Comment at: clang/test/CXX/dcl/dcl.fct/p17.cpp:261-279
+ template <typename T> struct S3 {};
+ // expected-note@-1 {{'S3' declared here}}
+ // expected-note@-2 {{template is declared here}}
+ // clang-format off
+ void f23(C2<::S3> auto);
+ // expected-error@-1 {{no template named 'S3' in the global namespace; did
you mean simply 'S3'?}}
+ // expected-error@-2 {{use of class template '::S3' requires template
arguments}}
----------------
So I think that the patch is just papering over the bug with another one of no
consequence in these tests.
If you look at this code in ASTContext.cpp around line 5695 on
getAutoTypeInternal:
```
bool AnyNonCanonArgs =
::getCanonicalTemplateArguments(*this, TypeConstraintArgs, CanonArgs);
if (AnyNonCanonArgs) {
Canon = getAutoTypeInternal(QualType(), Keyword, IsDependent, IsPack,
TypeConstraintConcept, CanonArgs, true);
// Find the insert position again.
AutoTypes.FindNodeOrInsertPos(ID, InsertPos);
}
```
Your patch makes AnyNonCanonArgs be false when it should be true, as structural
comparison of the argument to the canonical argument should indicate that the
argument is not canonical.
What is happening here on the real case, where `AnyNonCanonArgs = true`, is
that we end up outputting a NULL InsertPos on that "// Find the insert position
again." part, clearly because that `FindNodeOrInsertPos` actually found the
node.
But that should be impossible, because we clearly looked for it earlier and
would have returned before if so.
So that means that the call to create the canonical autotype somehow created
the same type as we want to create for the non-canonical one, which again is
bananas because the arguments are clearly not structurally identical as we
established.
So I think the only possibility here is that this is a profiling bug, we are
somehow not profiling template arguments correctly.
A quick look at `TemplateArgument::Profile` shows some things that look clearly
long, for example in the template case where we profile the canonical template
name instead of the as-written one. This is probably the bug that is hitting
these test cases.
There is also a problem in the Declaration case as well, where we are taking
the canonical declaration, this should presumably also cause a similar bug.
If that does fix it, can you also add tests which would have caught the bug
that this current patch would have introduced?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126172/new/
https://reviews.llvm.org/D126172
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
