[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-10 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane abandoned this revision.
erichkeane added a comment.

Superceded by https://reviews.llvm.org/D146178


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-07 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane updated this revision to Diff 511753.
erichkeane added a comment.

Made changes as requested by Richard.  "SkipForInstantiation" needed to be 
added when we added levels, but I think this is something that probably needs 
to be looked at and removed in the future.

I also see the change that this means to fix is reverted, so 
@alexander-shaposhnikov : feel free to take this and integrated it into your 
patch if you'd like.


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

https://reviews.llvm.org/D147722

Files:
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -825,3 +825,53 @@
   template U> friend constexpr auto decltype(L)::operator()() const;
 };
 } // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -131,6 +131,38 @@
   return Response::Done();
 }
 
+Response HandlePartialClassTemplateSpec(
+const ClassTemplatePartialSpecializationDecl *PartialClassTemplSpec,
+MultiLevelTemplateArgumentList , bool SkipForSpecialization) {
+  // We don't want the arguments from the Partial Specialization, since
+  // anything instantiating here cannot access the arguments from the
+  // specialized template anyway, so any substitution we would do with these
+  // partially specialized arguments would 'wrong' and confuse constraint
+  // instantiation. We only do this in the case of a constraint check, since
+  // code elsewhere actually uses these and replaces them later with what
+  // they mean.
+  // If we know this is the 'top level', we can replace this with an
+  // OuterRetainedLevel, else we have to generate a set of identity arguments.
+
+  // If this is the top-level template entity, we can just add a retained level
+  // and be done.
+  if (!PartialClassTemplSpec->getTemplateDepth()) {
+if (!SkipForSpecialization)
+  Result.addOuterRetainedLevel();
+return Response::Done();
+  }
+
+  // Else, we can replace this with an 'empty' level, and the checking will just
+  // alter the 'depth', since this we don't have the 'Index' for this level.
+  if (!SkipForSpecialization)
+Result.addOuterTemplateArguments(
+const_cast(
+PartialClassTemplSpec),
+{}, /*Final=*/false);
+
+  return Response::UseNextDecl(PartialClassTemplSpec);
+}
+
 // Add template arguments from a class template instantiation.
 Response
 HandleClassTemplateSpec(const ClassTemplateSpecializationDecl *ClassTemplSpec,
@@ -310,6 +342,10 @@
 if (const auto *VarTemplSpec =
 dyn_cast(CurDecl)) {
   R = HandleVarTemplateSpec(VarTemplSpec, Result, SkipForSpecialization);
+} else if (const auto *PartialClassTemplSpec =
+   dyn_cast(CurDecl)) {
+  R = HandlePartialClassTemplateSpec(PartialClassTemplSpec, Result,
+ SkipForSpecialization);
 } else if (const auto *ClassTemplSpec =
dyn_cast(CurDecl)) {
   R = HandleClassTemplateSpec(ClassTemplSpec, Result,
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -2854,7 +2854,7 @@
 template <>
 bool DeducedArgsNeedReplacement(
 ClassTemplatePartialSpecializationDecl *Spec) {
-  return !Spec->isClassScopeExplicitSpecialization();
+  return true;
 }
 
 template 
@@ -2881,7 +2881,7 @@
   // not class-scope explicit specialization, so replace with Deduced Args
   // instead of adding to inner-most.
   if (NeedsReplacement)
-MLTAL.replaceInnermostTemplateArguments(CanonicalDeducedArgs);
+MLTAL.replaceInnermostTemplateArguments(Template, CanonicalDeducedArgs);
 
   if (S.CheckConstraintSatisfaction(Template, AssociatedConstraints, MLTAL,

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-07 Thread Alexander Yermolovich via Phabricator via cfe-commits 
ayermolo added a comment.

OK was able to create small(ish) repro

main.h

  #include 
  enum class Enums {
  ACCESS = 0,
  MAP = 1,
  };
  
  template 
  class DMap {
   public:
  template 
  requires std::is_convertible_v
void getOrCreate(TIDR v);
  };
  
  template 
  class DMap {
   public:
  template 
  requires std::is_convertible_v
void getOrCreate(TIDR v);
  
  };

main.cpp

  #include "main.h"
  
  // broken with original patch, broken with follow up fix
  template 
  template 
  requires std::is_convertible_v
  inline void
  DMap::getOrCreate(TIDR v) {
  }
  
  // broken with original patch, fixed with follow up fix
  template 
  template 
requires std::is_convertible_v
  inline void
  DMap::getOrCreate(TIDR v) {
  }
  
  
  int foo() {
  DMap map;
  long i;
  map.getOrCreate(i);
  }


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-07 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added a comment.

I think it is probably a good idea to revert that one until you get a chance to 
figure out that problem (and #61993 if it is related!).  Other than the test 
being dependent on your change (and perhaps failing without your change), this 
is otherwise unrelated, so I might commit it anyway.


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Chuanqi Xu via Phabricator via cfe-commits 
ChuanqiXu added a comment.

In D147722#4250133 , 
@alexander-shaposhnikov wrote:

> fwiw - I can revert https://reviews.llvm.org/D146178 for now till we fix the 
> newly discovered cases (at the moment I'm aware of GH61959 and the one 
> reported by @ilya-biryukov. 
> Basically let me know what you think - i'm looking into the issue reported by 
> Ilya, but it will take time.

+1 since this breaks our codebases too.


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Alexander Yermolovich via Phabricator via cfe-commits 
ayermolo added a comment.

That would be great. :)


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Alexander Shaposhnikov via Phabricator via cfe-commits 
alexander-shaposhnikov added a subscriber: ilya-biryukov.
alexander-shaposhnikov added a comment.

fwiw - I can revert https://reviews.llvm.org/D146178 for now till we fix the 
newly discovered cases (at the moment I'm aware of GH61959 and the one reported 
by @ilya-biryukov. 
Basically let me know what you think - i'm looking into the issue reported by 
Ilya, but it will take time.


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Alexander Yermolovich via Phabricator via cfe-commits 
ayermolo added a comment.

With this patch here is still a build failure in our code base. I am trying to 
create a small repro. But from high level there is a templated class that has 
partial specializations, and it errors out on definition of a function that 
doesn't have any.


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D147722#4249612 , @erichkeane 
wrote:

> Hmmm... the examples I thought would cause a problem don't seem to when 
> switching that to Done.  So I'm going to run regression tests and give that a 
> shot.
>
> I think I now get what you mean about 'Identity' arguments, we basically need 
> to have arguments to keep something like the above example (where 'foo' is a 
> template, and the partial specialization is done correctly), and when trying 
> to substitute into it, we would end up having the arguments to `Outer` and 
> `Foo`, but substitution would think of the `Foo` args as the `Inner` args for 
> depth.  I'm giving the testing a shot, and will see if that fixes the problem 
> without causing a regression in any of the concerns I have.  I suspect I 
> don't have a problem with the 'identity' for the same reason 'Done' works: we 
> never hit that situation.

If we know that all further levels will be "identity" levels (mapping 
template-param-i-j to template-param-i-j), then we can use 
`MultiLevelTemplateArgumentList::addOuterRetainedLevels` to represent that and 
leave those levels alone. That will mean the `getNumSubstitutedLevels() == 0` 
check in `SubstituteConstraintExpression` will fire a lot more often, which 
seems like a nice benefit.


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added a comment.

The switching that to 'done' seems to have broken cases where the partial 
specialization itself has requires clauses on it, see 
https://github.com/llvm/llvm-project/blob/main/clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp#L75

Static Assert on 75 and 76 now fail with:

   error: 'error' diagnostics seen but not expected:
File 
/localdisk2/ekeane1/workspaces/llvm-project/clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
 Line 75: static assertion failed due to requirement 'S<1>::F::value == 2'
File 
/localdisk2/ekeane1/workspaces/llvm-project/clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
 Line 76: static assertion failed due to requirement 'S<1>::F::value 
== 3'
  error: 'note' diagnostics seen but not expected:
File 
/localdisk2/ekeane1/workspaces/llvm-project/clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
 Line 75: expression evaluates to '1 == 2'
File 
/localdisk2/ekeane1/workspaces/llvm-project/clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
 Line 76: expression evaluates to '1 == 3'
  4 errors generated.

AND 
https://github.com/llvm/llvm-project/blob/main/clang/test/SemaTemplate/concepts.cpp#L323

  error: 'error' diagnostics seen but not expected:
File 
/localdisk2/ekeane1/workspaces/llvm-project/clang/test/SemaTemplate/concepts.cpp
 Line 332: no type named 'type' in 
'DeducedTemplateArgs::ItrTraits::Ptr'
  error: 'note' diagnostics seen but not expected:
File 
/localdisk2/ekeane1/workspaces/llvm-project/clang/test/SemaTemplate/concepts.cpp
 Line 341: in instantiation of template class 
'DeducedTemplateArgs::ItrTraits' requested 
here
  2 errors generated.

So I think it is causing something to not be in the right order there 
(particularly the 1st one).  I'll look into the identity replacement of 
template args.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:291
 /// encountering a lambda generic call operator, and continue looking for
 /// arguments on an enclosing class template.
 

alexander-shaposhnikov wrote:
> /* not directly related to this patch, just one thought while we are here */ 
> it would be useful to add missing documentation (comments) for the last 
> parameter
> (SkipForSpecialization)
> p.s. perhaps, expanding the comment (or even adding some examples) would be 
> super helpful as well (for future readers)
I definitely agree.. I don't particularly understand what happened there, but I 
couldn't come up with an alternate fix/come up with a 'better' solution to the 
problem.  See original commit here: https://reviews.llvm.org/D134128



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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Alexander Shaposhnikov via Phabricator via cfe-commits 
alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:291
 /// encountering a lambda generic call operator, and continue looking for
 /// arguments on an enclosing class template.
 

/* not directly related to this patch, just one thought while we are here */ 
it would be useful to add missing documentation (comments) for the last 
parameter
(SkipForSpecialization)
p.s. perhaps, expanding the comment (or even adding some examples) would be 
super helpful as well (for future readers)


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added a comment.

Hmmm... the examples I thought would cause a problem don't seem to when 
switching that to Done.  So I'm going to run regression tests and give that a 
shot.

I think I now get what you mean about 'Identity' arguments, we basically need 
to have arguments to keep something like the above example (where 'foo' is a 
template, and the partial specialization is done correctly), and when trying to 
substitute into it, we would end up having the arguments to `Outer` and `Foo`, 
but substitution would think of the `Foo` args as the `Inner` args for depth.  
I'm giving the testing a shot, and will see if that fixes the problem without 
causing a regression in any of the concerns I have.  I suspect I don't have a 
problem with the 'identity' for the same reason 'Done' works: we never hit that 
situation.


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:322
+  // they mean.
+  R = Response::UseNextDecl(PartialClassTemplSpec);
 } else if (const auto *ClassTemplSpec =

erichkeane wrote:
> rsmith wrote:
> > Can we produce a "done" response instead here? It doesn't seem right to 
> > carry on to the next level here without adding a level of template 
> > arguments -- if any further arguments were somehow collected, they would be 
> > collected at  the wrong template depth. But, as far as I can determine, we 
> > should never encounter a partial specialization declaration except in cases 
> > where there is nothing remaining to be substituted (all outer substitutions 
> > would be identity substitutions). I think the same thing applies when we 
> > reach a class template declaration (the first branch in `HandleRecordDecl`) 
> > -- we should just be able to stop in that case too, rather than collecting 
> > some identity template arguments and carrying on to an outer level.
> > 
> > If we can't produce a "done" response here for some reason, then we should 
> > collect a level of identity template arguments instead.
> For constraints, the arguments are relative to namespace scope as far as I 
> can tell.  If we ended up 'stopping' we would miss references to the outer 
> template, so something like:
> 
> ``` 
> template
> struct Outer {
> 
>   template
>   struct Inner {
>  void foo() requires (is_same){}
>   };
> 
> };
> 
> ```
> 
> Right?  The substitution fo 'T' would be invalid at that point?
> 
> Also, what do you mean by 'identity' template arguments?  I'm not sure I 
> follow.
Er, I meant something like (since it needs to be a partial spec): 
```
  1 template
  2 concept C = true;
  3
  4 template
  5 struct Outer {
  6   template
  7   struct Inner {};
  8   template
  9   struct Inner {
 10 void foo()  requires C && C{}
 11 void foo()  requires true{}
 12   };
 13 };
```


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2854-2858
-template <>
-bool DeducedArgsNeedReplacement(
-ClassTemplatePartialSpecializationDecl *Spec) {
-  return !Spec->isClassScopeExplicitSpecialization();
-}

rsmith wrote:
> It's not obvious to me what this was doing, so I'm not sure whether it's 
> correct to remove it. Can you explain? It makes me uncomfortable that we 
> would treat class template partial specializations and variable template 
> partial specializations differently, at least without a good explanation for 
> why that's appropriate -- perhaps we should apply this same set of changes to 
> variable template partial specializations too, and remove this mechanism 
> entirely?
Absolutely!  And I suspect at a certain point we are going to want to remove 
this entirely, it is a bit of a hack (and I couldn't come up with a good repro 
that this caused a problem, and the `VarTemplateSpecializationDecl` code does a 
lot of work).

  
Basically, when doing the `CheckDeducedArgumentConstraints` below, we get the 
instantiation args list (see this being used on 2869).  

However, the 'inner most' args when this is a Partial Specialization end up 
being 'wrong', since they come from the partial specialization.  This is used 
to determine whether we need to replace them with the args we already have. 
However, this patch now makes it so `ClassTemplatePartialSpecializationDecl`s 
no longer generate template arguments in `getTemplateInstantiationArgs` (which 
has seen a bit of a refactor since you were here last, since it was a bit of a 
difficult to understand function that didn't seem to cover many cases well, and 
had a lot of repetation).





Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:322
+  // they mean.
+  R = Response::UseNextDecl(PartialClassTemplSpec);
 } else if (const auto *ClassTemplSpec =

rsmith wrote:
> Can we produce a "done" response instead here? It doesn't seem right to carry 
> on to the next level here without adding a level of template arguments -- if 
> any further arguments were somehow collected, they would be collected at  the 
> wrong template depth. But, as far as I can determine, we should never 
> encounter a partial specialization declaration except in cases where there is 
> nothing remaining to be substituted (all outer substitutions would be 
> identity substitutions). I think the same thing applies when we reach a class 
> template declaration (the first branch in `HandleRecordDecl`) -- we should 
> just be able to stop in that case too, rather than collecting some identity 
> template arguments and carrying on to an outer level.
> 
> If we can't produce a "done" response here for some reason, then we should 
> collect a level of identity template arguments instead.
For constraints, the arguments are relative to namespace scope as far as I can 
tell.  If we ended up 'stopping' we would miss references to the outer 
template, so something like:

``` 
template
struct Outer {

  template
  struct Inner {
 void foo() requires (is_same){}
  };

};

```

Right?  The substitution fo 'T' would be invalid at that point?

Also, what do you mean by 'identity' template arguments?  I'm not sure I follow.


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2854-2858
-template <>
-bool DeducedArgsNeedReplacement(
-ClassTemplatePartialSpecializationDecl *Spec) {
-  return !Spec->isClassScopeExplicitSpecialization();
-}

It's not obvious to me what this was doing, so I'm not sure whether it's 
correct to remove it. Can you explain? It makes me uncomfortable that we would 
treat class template partial specializations and variable template partial 
specializations differently, at least without a good explanation for why that's 
appropriate -- perhaps we should apply this same set of changes to variable 
template partial specializations too, and remove this mechanism entirely?



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:318
+  // specialized template anyway, so any substitution we would do with 
these
+  // partially specialized arguments would 'wrong' and confuse constraint
+  // instantiation. We only do this in the case of a constraint check, 
since





Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:322
+  // they mean.
+  R = Response::UseNextDecl(PartialClassTemplSpec);
 } else if (const auto *ClassTemplSpec =

Can we produce a "done" response instead here? It doesn't seem right to carry 
on to the next level here without adding a level of template arguments -- if 
any further arguments were somehow collected, they would be collected at  the 
wrong template depth. But, as far as I can determine, we should never encounter 
a partial specialization declaration except in cases where there is nothing 
remaining to be substituted (all outer substitutions would be identity 
substitutions). I think the same thing applies when we reach a class template 
declaration (the first branch in `HandleRecordDecl`) -- we should just be able 
to stop in that case too, rather than collecting some identity template 
arguments and carrying on to an outer level.

If we can't produce a "done" response here for some reason, then we should 
collect a level of identity template arguments instead.


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

https://reviews.llvm.org/D147722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147722: [Concepts] Fix Function Template Concepts comparisons

2023-04-06 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane created this revision.
erichkeane added reviewers: clang-language-wg, alexander-shaposhnikov.
Herald added a project: All.
erichkeane requested review of this revision.

As reported in GH61959, the patch for https://reviews.llvm.org/D146178
 regressed some comparisons of non-out-of-line function constraints.
 This patch fixes it by making sure we skip the list of template
 arguments from a partial specialization (where they don't really
 matter!).

There was 1 other workaround that was made for checking deduced
arguments that this removes part of.

Fixes: #61959


https://reviews.llvm.org/D147722

Files:
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/concepts.cpp


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -825,3 +825,34 @@
   template U> friend constexpr auto decltype(L)::operator()() 
const;
 };
 } // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -310,6 +310,16 @@
 if (const auto *VarTemplSpec =
 dyn_cast(CurDecl)) {
   R = HandleVarTemplateSpec(VarTemplSpec, Result, SkipForSpecialization);
+} else if (const auto *PartialClassTemplSpec =
+   dyn_cast(CurDecl)) {
+  // We don't want the arguments from the Partial Specialization, since
+  // anything instantiating here cannot access the arguments from the
+  // specialized template anyway, so any substitution we would do with 
these
+  // partially specialized arguments would 'wrong' and confuse constraint
+  // instantiation. We only do this in the case of a constraint check, 
since
+  // code elsewhere actually uses these and replaces them later with what
+  // they mean.
+  R = Response::UseNextDecl(PartialClassTemplSpec);
 } else if (const auto *ClassTemplSpec =
dyn_cast(CurDecl)) {
   R = HandleClassTemplateSpec(ClassTemplSpec, Result,
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -2851,11 +2851,6 @@
 VarTemplatePartialSpecializationDecl *Spec) {
   return !Spec->isClassScopeExplicitSpecialization();
 }
-template <>
-bool DeducedArgsNeedReplacement(
-ClassTemplatePartialSpecializationDecl *Spec) {
-  return !Spec->isClassScopeExplicitSpecialization();
-}
 
 template 
 static Sema::TemplateDeductionResult


Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -825,3 +825,34 @@
   template U> friend constexpr auto decltype(L)::operator()() const;
 };
 } // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -310,6 +310,16 @@
 if (const auto *VarTemplSpec =
 dyn_cast(CurDecl)) {
   R = HandleVarTemplateSpec(VarTemplSpec, Result, SkipForSpecialization);
+} else if (const auto *PartialClassTemplSpec =
+   dyn_cast(CurDecl)) {
+  // We don't want the arguments from the Partial Specialization, since
+  // anything instantiating here cannot access the arguments from the
+  // specialized template anyway, so any substitution we would do with these
+  // partially specialized arguments would 'wrong' and confuse constraint
+  // instantiation. We only do this in the case of a constraint check, since
+  // code elsewhere actually uses