[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D126172#3767188 , @roberteg16 
wrote:

> Hey @mizvekov! Sorry for the late response.
>
> Yes, I am still interested and I am working on this. I'll let you know ASAP 
> if I find something that resembles a valid fix.
>
> I would be very grateful if you could give me some tips for debuging better 
> clang or if you could point me towards documents, links, whatever that could 
> give me nice information about how to reason about the code of the 
> parser/sema of clang.
>
> If you are in a hurry and need to fix it yourself and it happens that you 
> finish the path first, please @ me so I can get to see how an error like this 
> would be fixed, thanks!

Hello! Yeah you were just a bit too late, I have a patch that is accepted and 
should go in soon: https://reviews.llvm.org/D133072
I had run into this problem while making changes around 
CheckTemplateArgumentList, and I have this big patch which I am trying to 
offload into smaller chunks to expedite review.

I don't have any documents to recommend besides the obvious things, like the 
Clang docs auto-generated from the source code.
If you do want to get started, I can only recommend what worked for me, which 
is to try fixing bugs first, much like you tried here.
Though perhaps it's not very productive to get stuck in any one for a very long 
time, instead just ask for help or move on :)

I think that if you want to implement something new or a more substantial 
change, being able to debug quickly certainly helps.
There is no way you are going to be able to understand what is going on early 
on, so you will make a big mess, break hundreds of tests and so on, and being 
able to quickly go over and understand what is going on with them is important.
Invest in isolating bugs before working on them, it always pays off.

When fixing stuff, beware of changing assertions, most of the times the 
assertion is correct.
Also, if a bug fix is too local and seems easy, it's always worth spending some 
extra time to be sure.

And lots and lots and lots of free time, patience and being able to work 
without distractions.


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

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 added a comment.

In D126172#3754221 , @mizvekov wrote:

> Hello, @roberteg16, are you still interested in working on this patch?
>
> I might need a fix for this myself, and also it happened recently that 
> someone else attempted a fix for the same bug.
>
> If you are not abandoning it, please let us know!

Hey @mizvekov! Sorry for the late response.

Yes, I am still interested and I am working on this. I'll let you know ASAP if 
I find something that resembles a valid fix.

I would be very grateful if you could give me some tips for debuging better 
clang or if you could point me towards documents, links, whatever that could 
give me nice information about how to reason about the code of the parser/sema 
of clang.

If you are in a hurry and need to fix it yourself and it happens that you 
finish the path first, please @ me so I can get to see how an error like this 
would be fixed, thanks!


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

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

Hello, @roberteg16, are you still interested in working on this patch?

I might need a fix for this myself, and also it happened recently that someone 
else attempted a fix for the same bug.

If you are not abandoning it, please let us know!


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

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a subscriber: rjmccall.
mizvekov added a comment.

In D126172#3626809 , @roberteg16 
wrote:

> Hi again @mizvekov, I've been spending a bit of time on resuming this issue.
>
> When applying (after reverting the changes) the first change related to the 
> `Template` case of profiling the as-written one name this resulted in the 
> following test failing:
> 
> I've been looking around trying to figure out what's the issue but I am 
> feeling that I lack the required knowledge to diagnose properly the problem.

I think we may be failing to canonicalize template arguments somewhere else 
where it's required.

We could be for example producing two different types because we produced two 
different template specializations for the same canonical template arguments.

The most likely suspect is `Sema::CheckTemplateArgumentList` in 
`SemaTemplate.cpp`.
That function is supposed to receive the arguments as written as input, and 
then produce a 'resolved' template argument list in the 'Converted' output 
parameter.

Looking at the helper function `CheckTemplateArgument`, right at the bottom for 
the Template and TemplateExpansion cases, we are pushing the argument into the 
Converted list without canonicalizing it.
I think that is one problem.

Back at `CheckTemplateArgumentList`, there are also some cases where we are 
perfoming pack expansions where we are potentially pushing non-canonical 
template arguments into that list, this is also worth looking over.

> One question I want to ask is: On `StmtProfiler::VisitTemplateArgument` it 
> states on a comment:
>
>> // Mostly repetitive with TemplateArgument::Profile!
>
> but as far as I could see it really does not mimic the behavior in some 
> cases. Is this at purpose?
>
> Thanks for your time.

That one looks mostly correct, except perhaps that we should be profiling the 
number of known expansions for the TemplateExpansion case, but that goes for 
both implementations.

I don't know if this was done on purpose but then became invalid later with 
other changes, the commits which introduces these things are pretty ancient, 
they date back to SVN era, and there is a lack of explanations in the history.

We can try pinging whoever was involved at the time. I can't find the phab 
handle of the author of that commit, but it seems @rjmccall might have been 
involved and he is still around.


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

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 added a comment.

Hi again @mizvekov, I've been spending a bit of time on resuming this issue.

When applying (after reverting the changes) the first change related to the 
`Template` case of profiling the as-written one name this resulted in the 
following test failing:

Changes:

  -ID.AddPointer(Context.getCanonicalTemplateName(Template)
  .getAsVoidPointer());
  +ID.AddPointer(Template.getAsVoidPointer());


Test failing:

  Clang :: 
CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp
  Clang :: CXX/temp/temp.decls/temp.variadic/multi-level-substitution.cpp
  Clang :: CodeGenCXX/mangle-variadic-templates.cpp
  Clang :: CodeGenCXX/mangle.cpp

The main reason of the failure is that now clang outputs errors. I'll only copy 
the CodeGenCXX/mangle.cpp output:

  /home/robert/Documents/llvm-project/clang/test/CodeGenCXX/mangle.cpp:516:17: 
error: explicit instantiation of 'foo' does not refer to a function template, 
variable template, member function, member class, or static data member
template void foo(const A &a);
  ^
  /home/robert/Documents/llvm-project/clang/test/CodeGenCXX/mangle.cpp:513:43: 
note: candidate template ignored: failed template argument deduction
template  class T> void foo(const A &a) {}
^
  /home/robert/Documents/llvm-project/clang/test/CodeGenCXX/mangle.cpp:1146:10: 
warning: decomposition declarations are a C++17 extension [-Wc++17-extensions]
  auto [e] = g;
   ^~~
  /home/robert/Documents/llvm-project/clang/test/CodeGenCXX/mangle.cpp:1153:8: 
warning: decomposition declarations are a C++17 extension [-Wc++17-extensions]
auto [a,b] = X{1,2};

I've been looking around trying to figure out what's the issue but I am feeling 
that I lack the required knowledge to diagnose properly the problem.

One question I want to ask is: On `StmtProfiler::VisitTemplateArgument` it 
states on a comment:

> // Mostly repetitive with TemplateArgument::Profile!

but as far as I could see it really does not mimic the behavior in some cases. 
Is this at purpose?

Thanks for your time.


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

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 added a comment.

Thanks @mizvekov for such an insightful review. I'll try to address ASAP 
everything you said and let you know the results.


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

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Matheus Izvekov via Phabricator via cfe-commits]

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  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

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 added a comment.

@rsmith polite ping


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

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 updated this revision to Diff 431451.
roberteg16 added a comment.

Fix wrongly moved clang-format disabling comment when trying to fix clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126172/new/

https://reviews.llvm.org/D126172

Files:
  clang/lib/AST/TemplateBase.cpp
  clang/test/CXX/dcl/dcl.fct/p17.cpp


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+  template  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}}
+  // clang-format on
+  } // namespace constrained
+
+  template  struct S4 {};
+  // expected-note@-1 {{template is declared here}}
+
+  namespace constrained {
+  // clang-format off
+  void f24(C2<::S4> auto);
+  // expected-error@-1 {{use of class template '::S4' requires template 
arguments}}
+  // clang-format on
+  } // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,7 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+  template  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}}
+  // clang-format on
+  } // namespace constrained
+
+  template  struct S4 {};
+  // expected-note@-1 {{template is declared here}}
+
+  namespace constrained {
+  // clang-format off
+  void f24(C2<::S4> auto);
+  // expected-error@-1 {{use of class template '::S4' requires template arguments}}
+  // clang-format on
+  } // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,7 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 updated this revision to Diff 431416.
roberteg16 added a comment.

Fix clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126172/new/

https://reviews.llvm.org/D126172

Files:
  clang/lib/AST/TemplateBase.cpp
  clang/test/CXX/dcl/dcl.fct/p17.cpp


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+  template  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  void f23(C2<::S3> auto);
+  // clang-format off
+  // 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}}
+  // clang-format on
+  } // namespace constrained
+
+  template  struct S4 {};
+  // expected-note@-1 {{template is declared here}}
+
+  namespace constrained {
+  void f24(C2<::S4> auto);
+  // clang-format off
+  // expected-error@-1 {{use of class template '::S4' requires template 
arguments}}
+  // clang-format on
+  } // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,7 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+  template  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  void f23(C2<::S3> auto);
+  // clang-format off
+  // 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}}
+  // clang-format on
+  } // namespace constrained
+
+  template  struct S4 {};
+  // expected-note@-1 {{template is declared here}}
+
+  namespace constrained {
+  void f24(C2<::S4> auto);
+  // clang-format off
+  // expected-error@-1 {{use of class template '::S4' requires template arguments}}
+  // clang-format on
+  } // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,7 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 updated this revision to Diff 431389.
roberteg16 added a comment.

Try fix clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126172/new/

https://reviews.llvm.org/D126172

Files:
  clang/lib/AST/TemplateBase.cpp
  clang/test/CXX/dcl/dcl.fct/p17.cpp


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+// clang-format off
+  template  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  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}}
+// clang-format on
+} // namespace constrained
+
+template  struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+// clang-format off
+void f24(C2<::S4> auto);
+// expected-error@-1 {{use of class template '::S4' requires template 
arguments}}
+// clang-format on
+} // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,7 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+// clang-format off
+  template  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  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}}
+// clang-format on
+} // namespace constrained
+
+template  struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+// clang-format off
+void f24(C2<::S4> auto);
+// expected-error@-1 {{use of class template '::S4' requires template arguments}}
+// clang-format on
+} // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,7 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 updated this revision to Diff 431356.
roberteg16 added a comment.

Stricten comparison


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126172/new/

https://reviews.llvm.org/D126172

Files:
  clang/lib/AST/TemplateBase.cpp
  clang/test/CXX/dcl/dcl.fct/p17.cpp


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+  // clang-format off
+  template  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  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}}
+  // clang-format on
+} // namespace constrained
+
+template  struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+// clang-format off
+void f24(C2<::S4> auto);
+// expected-error@-1 {{use of class template '::S4' requires template 
arguments}}
+// clang-format on
+} // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,7 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+  // clang-format off
+  template  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  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}}
+  // clang-format on
+} // namespace constrained
+
+template  struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+// clang-format off
+void f24(C2<::S4> auto);
+// expected-error@-1 {{use of class template '::S4' requires template arguments}}
+// clang-format on
+} // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,7 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl() &&
TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
 
   case Declaration:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 updated this revision to Diff 431264.
roberteg16 edited the summary of this revision.
roberteg16 added a comment.

Make clang-format not complain


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126172/new/

https://reviews.llvm.org/D126172

Files:
  clang/lib/AST/TemplateBase.cpp
  clang/test/CXX/dcl/dcl.fct/p17.cpp


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+  // clang-format off
+  template  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  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}}
+  // clang-format on
+} // namespace constrained
+
+template  struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+// clang-format off
+void f24(C2<::S4> auto);
+// expected-error@-1 {{use of class template '::S4' requires template 
arguments}}
+// clang-format on
+} // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,8 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
-   TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl();
 
   case Declaration:
 return getAsDecl() == Other.getAsDecl();


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,23 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
-}
+
+  // clang-format off
+  template  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  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}}
+  // clang-format on
+} // namespace constrained
+
+template  struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+// clang-format off
+void f24(C2<::S4> auto);
+// expected-error@-1 {{use of class template '::S4' requires template arguments}}
+// clang-format on
+} // namespace constrained
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,8 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
-   TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl();
 
   case Declaration:
 return getAsDecl() == Other.getAsDecl();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126172: [clang] Fix comparison of TemplateArgument when they are of template kind

[Robert Esclapez via Phabricator via cfe-commits]

roberteg16 created this revision.
roberteg16 added reviewers: Richard, smith.
Herald added a project: All.
roberteg16 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This diff path takes care of comparing Template{Expansion} by template decl
instead of by comparing the ptr attr Name and the number of expansions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126172

Files:
  clang/lib/AST/TemplateBase.cpp
  clang/test/CXX/dcl/dcl.fct/p17.cpp


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,21 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
+
+  template 
+  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  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}}
+}
+
+template 
+struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+  void f24(C2<::S4> auto);
+  // expected-error@-1 {{use of class template '::S4' requires template 
arguments}}
 }
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,8 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
-   TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl();
 
   case Declaration:
 return getAsDecl() == Other.getAsDecl();


Index: clang/test/CXX/dcl/dcl.fct/p17.cpp
===
--- clang/test/CXX/dcl/dcl.fct/p17.cpp
+++ clang/test/CXX/dcl/dcl.fct/p17.cpp
@@ -257,4 +257,21 @@
   static_assert(is_same_v);
   // expected-error@-1{{no matching}}
   static_assert(is_same_v);
+
+  template 
+  struct S3 {};
+  // expected-note@-1 {{'S3' declared here}}
+  // expected-note@-2 {{template is declared here}}
+  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}}
+}
+
+template 
+struct S4 {};
+// expected-note@-1 {{template is declared here}}
+
+namespace constrained {
+  void f24(C2<::S4> auto);
+  // expected-error@-1 {{use of class template '::S4' requires template arguments}}
 }
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -370,8 +370,8 @@
 
   case Template:
   case TemplateExpansion:
-return TemplateArg.Name == Other.TemplateArg.Name &&
-   TemplateArg.NumExpansions == Other.TemplateArg.NumExpansions;
+return getAsTemplateOrTemplatePattern().getAsTemplateDecl() ==
+   Other.getAsTemplateOrTemplatePattern().getAsTemplateDecl();
 
   case Declaration:
 return getAsDecl() == Other.getAsDecl();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits