[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D119544#3527556 , @erichkeane 
wrote:

> In D119544#3526810 , @ChuanqiXu 
> wrote:
>
>> In D119544#3494281 , @erichkeane 
>> wrote:
>>
>>> Updated to include the fix for the libcxx issue, which was that we weren't 
>>> properly differentiating between 'friend' functions based on concepts.  
>>> I'll likely be submitting this early monday or so, but would like to give 
>>> @ChuanqiXu  a chance to check out the changes.
>>>
>>> This is the same as my 'last' committed version of this patch, plus the 
>>> changes I split out into here for easier review: 
>>> https://reviews.llvm.org/D125020
>>
>> Sorry that I missed the ping. The change in that revision looks good to me.
>>
>>> I THINK what we want to do instead of trying to 're setup' the template 
>>> arguments (and instead of just 'keeping' the uninstantiated expression) is 
>>> to run some level of 'tree-transform' that updates the names of the 
>>> template parameters (including depths/etc), but without doing lookup. This 
>>> I think would fix our 'friend function' issue, and our 'comparing 
>>> constraints' stuff work correctly as well (rather than storing an 
>>> instantiated version, but not reusing it except for comparing them).
>>
>> I agree this should be a better direction.
>
> I actually got a response from Richard who seems to be more in favor of the 
> solution I tried initially (the one in this patch!).  The problems I have 
> with it I think get solved by the 'friend function' rules that I pasted 
> above, so I THINK I can fix those and be ok.  I'll still need SOME level of 
> tree-transform, but only to see if it depends on the enclosing template.

Great!

>> I agree this one should be fine. I think we could fix the problem by making 
>> non-template function inside a template class to be a dependent type. Like 
>> this one is accepted: >https://godbolt.org/z/MorzcqE1a
>> This bug might not be horrible since few developer would write friend 
>> function definition which doesn't touch the enclosing class.
>
> Yeah, that is a really strange one.  I don't think we can make it dependent 
> as then it wouldn't be callable when fully instantiate-able.  I don't have a 
> good idea how to make this work in the AST though, and might end up leaving 
> it as a 'fixme'.  As you said, I think you're right that this is a very small 
> bug-vector.

Yeah, my point is that this is not your fault. And we could fix the problem 
later when we have time since it is not hurry.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3526810 , @ChuanqiXu wrote:

> In D119544#3494281 , @erichkeane 
> wrote:
>
>> Updated to include the fix for the libcxx issue, which was that we weren't 
>> properly differentiating between 'friend' functions based on concepts.  I'll 
>> likely be submitting this early monday or so, but would like to give 
>> @ChuanqiXu  a chance to check out the changes.
>>
>> This is the same as my 'last' committed version of this patch, plus the 
>> changes I split out into here for easier review: 
>> https://reviews.llvm.org/D125020
>
> Sorry that I missed the ping. The change in that revision looks good to me.
>
>> I THINK what we want to do instead of trying to 're setup' the template 
>> arguments (and instead of just 'keeping' the uninstantiated expression) is 
>> to run some level of 'tree-transform' that updates the names of the template 
>> parameters (including depths/etc), but without doing lookup. This I think 
>> would fix our 'friend function' issue, and our 'comparing constraints' stuff 
>> work correctly as well (rather than storing an instantiated version, but not 
>> reusing it except for comparing them).
>
> I agree this should be a better direction.

I actually got a response from Richard who seems to be more in favor of the 
solution I tried initially (the one in this patch!).  The problems I have with 
it I think get solved by the 'friend function' rules that I pasted above, so I 
THINK I can fix those and be ok.  I'll still need SOME level of tree-transform, 
but only to see if it depends on the enclosing template.

> I agree this one should be fine. I think we could fix the problem by making 
> non-template function inside a template class to be a dependent type. Like 
> this one is accepted: >https://godbolt.org/z/MorzcqE1a
> This bug might not be horrible since few developer would write friend 
> function definition which doesn't touch the enclosing class.

Yeah, that is a really strange one.  I don't think we can make it dependent as 
then it wouldn't be callable when fully instantiate-able.  I don't have a good 
idea how to make this work in the AST though, and might end up leaving it as a 
'fixme'.  As you said, I think you're right that this is a very small 
bug-vector.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D119544#3494281 , @erichkeane 
wrote:

> Updated to include the fix for the libcxx issue, which was that we weren't 
> properly differentiating between 'friend' functions based on concepts.  I'll 
> likely be submitting this early monday or so, but would like to give 
> @ChuanqiXu  a chance to check out the changes.
>
> This is the same as my 'last' committed version of this patch, plus the 
> changes I split out into here for easier review: 
> https://reviews.llvm.org/D125020

Sorry that I missed the ping. The change in that revision looks good to me.

> I THINK what we want to do instead of trying to 're setup' the template 
> arguments (and instead of just 'keeping' the uninstantiated expression) is to 
> run some level of 'tree-transform' that updates the names of the template 
> parameters (including depths/etc), but without doing lookup. This I think 
> would fix our 'friend function' issue, and our 'comparing constraints' stuff 
> work correctly as well (rather than storing an instantiated version, but not 
> reusing it except for comparing them).

I agree this should be a better direction.

In D119544#3524844 , @erichkeane 
wrote:

> @rsmith pointed out https://eel.is/c++draft/temp#friend-9 which I think is 
> supposed to fix the friend function issues:
>
>> A non-template friend declaration with a requires-clause shall be a 
>> definition.
>> A friend function template with a constraint that depends on a template 
>> parameter from an enclosing template shall be a definition.
>> Such a constrained friend function or function template declaration does not 
>> declare the same function or function template as a declaration in any other 
>> scope.
>
> To me, this says that
>
>   template
>   struct S {
>   friend void foo() requires true {}
>   };
>   void bar() {
>   S s;
>   S s2;
>   }
>
> Should be perfectly fine, and NOT a redefinition?  At least this: 
> https://godbolt.org/z/PaK1Yhn1E seems to show that all 3 compilers get this 
> wrong currently?  Or I'm misreading this.

I agree this one should be fine. I think we could fix the problem by making 
non-template function inside a template class to be a dependent type. Like this 
one is accepted: https://godbolt.org/z/MorzcqE1a
This bug might not be horrible since few developer would write friend function 
definition which doesn't touch the enclosing class.

> It ALSO means that:
>
>   template
>   struct S {
>   template
>   friend void foo() requires constriant {}
>   };
>   void bar() {
>   S s;
>   S s2;
>   }
>
> is perfectly fine, and not a redefinition. (CURRENT Clang and GCC get this 
> right, but MSVC seems to get it wrong: https://godbolt.org/z/a7dYezoPW)
>
> HOWEVER, By reading the rule, a slightly DIFFERENT version of that
>
>   template
>   struct S {
>   template
>   friend void foo() requires constriant {} // No longer relies on the 
> enclosing template
>   };
>   void bar() {
>   S s;
>   S s2;
>   }
>
> Is a redefinition.  All 3 compilers diagnose this. 
> https://godbolt.org/z/7qbYsb635

I agree with the two judgement here. It looks like the implementation of clang 
is right on these two versions here.




Comment at: clang/lib/Sema/SemaConcept.cpp:169
   //operand is satisfied.
-  return false;
+  return LHSRes.isUsable() ? BO.recreateBinOp(S, LHSRes) : ExprEmpty();
 





Comment at: clang/lib/Sema/SemaConcept.cpp:178
   //the second operand is satisfied.
-  return false;
+  return LHSRes.isUsable() ? BO.recreateBinOp(S, LHSRes) : ExprEmpty();
 





Comment at: clang/lib/Sema/SemaConcept.cpp:185-186
+
+if (!LHSRes.isUsable() || !RHSRes.isUsable())
+  return ExprEmpty();
+return BO.recreateBinOp(S, LHSRes, RHSRes);





Comment at: clang/lib/Sema/SemaConcept.cpp:70
+assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good expressions?");
+assert(LHS.isUsable() && RHS.isUsable() && "Side not usable?");
+// We should just be able to 'normalize' these to the builtin Binary

erichkeane wrote:
> ChuanqiXu wrote:
> > I feel like the usage of the API could be further simplified.
> Heh, the suggestion is inverted slightly (those should be `!isUsable`) which 
> caught me for a while :)  Either way, I think it is a good suggestion.
Oh, my bad. It is a typo.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.
Herald added a reviewer: dang.

@rsmith pointed out https://eel.is/c++draft/temp#friend-9 which I think is 
supposed to fix the friend function issues:

> A non-template friend declaration with a requires-clause shall be a 
> definition.
> A friend function template with a constraint that depends on a template 
> parameter from an enclosing template shall be a definition.
> Such a constrained friend function or function template declaration does not 
> declare the same function or function template as a declaration in any other 
> scope.

To me, this says that

  template
  struct S {
  friend void foo() requires true {}
  };
  void bar() {
  S s;
  S s2;
  }

Should be perfectly fine, and NOT a redefinition?  At least this: 
https://godbolt.org/z/PaK1Yhn1E seems to show that all 3 compilers get this 
wrong currently?  Or I'm misreading this.

It ALSO means that:

  template
  struct S {
  template
  friend void foo() requires constriant {}
  };
  void bar() {
  S s;
  S s2;
  }

is perfectly fine, and not a redefinition. (CURRENT Clang and GCC get this 
right, but MSVC seems to get it wrong: https://godbolt.org/z/a7dYezoPW)

HOWEVER, By reading the rule, a slightly DIFFERENT version of that

  template
  struct S {
  template
  friend void foo() requires constriant {} // No longer relies on the 
enclosing template
  };
  void bar() {
  S s;
  S s2;
  }

Is a redefinition.  All 3 compilers diagnose this. 
https://godbolt.org/z/7qbYsb635

Do I have this right?  I'm having trouble figuring out the implementation 
strategy here.  It seems my SemaOverload.cpp changes would need to do special 
work to get example #1 to work, and I got close on #2 (with the exception of 
some assert in libc++).  And #3  I probably had right?

Can someone else read the standard bit there and make sure I got a good spread 
of the issues and interpreted the prose correctly?

Thanks!
-Erich


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I ended up having to revert again after my attempt this morning, this time due 
to some crash compiling a libc++ example.  I haven't been able to repro it yet, 
but I'm hopeful that one of follow-up buildbots will give me a better 
reproducer command line.

That said, I have a pretty heavy feeling that my approach here is perhaps 
misguided.  I attempted to follow the implementation of noexcept for this, but 
I think that was an incorrect direction.  I am going to try to work on a 
replacement for this using the same test changes that I've discovered/added 
through this patch, and see if my replacement is a better fit/idea.

I THINK what we want to do instead of trying to 're setup' the template 
arguments (and instead of just 'keeping' the uninstantiated expression) is to 
run some level of 'tree-transform' that updates the names of the template 
parameters (including depths/etc), but without doing lookup.  This  I think 
would fix our 'friend function' issue, and our 'comparing constraints' stuff 
work correctly as well (rather than storing an instantiated version, but not 
reusing it except for comparing them).

I'm unsure what fallout this will cause otherwise, but I think is worth the 
diversion.  Sadly, this might end up delaying our ability to compile 
libstdc++'s ranges for another release.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 427359.
erichkeane added a comment.

Updated to include the fix for the libcxx issue, which was that we weren't 
properly differentiating between 'friend' functions based on concepts.  I'll 
likely be submitting this early monday or so, but would like to give @ChuanqiXu 
 a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes 
I split out into here for easier review: https://reviews.llvm.org/D125020


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -Wno-unused-value -verify
 
 template  requires ((sizeof(Args) == 1), ...)
 // expected-note@-1 {{because 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I was able to find a reproduction of the problem:

template
  struct is_same { static constexpr bool value = false; };
template
  struct is_same { static constexpr bool value = false; };
  
  template
concept same_as = is_same::value;
  
  template 
  struct __range_adaptor_closure {
  
  
  template 
  requires same_as<_Tp, _Closure>
  friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
__closure){};
  
  };
  
  
  struct A : __range_adaptor_closure{};
  struct B : __range_adaptor_closure{};

As far as I can tell, there is nothing wrong with the libc++ code, so I have to 
figure out why after this patch that the requires clause doesn't realize these 
are different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3486573 , @gulfem wrote:

>> Then pipe that to a file (note the -E I added at the end).  You should get a 
>> file that looks like some slightly-wonky C++ code.
>
> I got the following output after running it via `-E`.
>
>   
> /usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:15:10:
>  fatal error: 'memory' file not found
>   #include 
>^~~~
>   # 1 
> "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
>   # 1 "" 1
>   # 1 "" 3
>   # 434 "" 3
>   # 1 "" 1
>   # 1 "" 2
>   # 1 
> "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
>  2
>   # 21 
> "/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
>   template 
>   struct ForwardProxyIterator {
> using value_type = int;
> using difference_type = int;
> ForwardProxyIterator& operator++();
> ForwardProxyIterator operator++(int);
> bool operator==(const ForwardProxyIterator&) const;
>   
> int operator*() const;
>   };
>   
>   
> static_assert(std::ranges::__nothrow_forward_range>);
>   
> static_assert(!std::ranges::__nothrow_forward_range>);
>   static_assert(std::ranges::forward_range>);
>   
> static_assert(!std::ranges::__nothrow_forward_range>);
>   
>   constexpr bool forward_subsumes_input(std::ranges::__nothrow_forward_range 
> auto&&) {
> return true;
>   }
>   constexpr bool forward_subsumes_input(std::ranges::__nothrow_input_range 
> auto&&);
>   
>   static_assert(forward_subsumes_input("foo"));
>   1 error generated.
>
> Would that be enough?

Ah, thats unfortunate, I wouldn't expect that to happen in the same place as 
the previous repo.  I'll see if I can find another way to repro it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> Then pipe that to a file (note the -E I added at the end).  You should get a 
> file that looks like some slightly-wonky C++ code.

I got the following output after running it via `-E`.

  
/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:15:10:
 fatal error: 'memory' file not found
  #include 
   ^~~~
  # 1 
"/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
  # 1 "" 1
  # 1 "" 3
  # 434 "" 3
  # 1 "" 1
  # 1 "" 2
  # 1 
"/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
 2
  # 21 
"/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
  template 
  struct ForwardProxyIterator {
using value_type = int;
using difference_type = int;
ForwardProxyIterator& operator++();
ForwardProxyIterator operator++(int);
bool operator==(const ForwardProxyIterator&) const;
  
int operator*() const;
  };
  
  
static_assert(std::ranges::__nothrow_forward_range>);
  
static_assert(!std::ranges::__nothrow_forward_range>);
  static_assert(std::ranges::forward_range>);
  
static_assert(!std::ranges::__nothrow_forward_range>);
  
  constexpr bool forward_subsumes_input(std::ranges::__nothrow_forward_range 
auto&&) {
return true;
  }
  constexpr bool forward_subsumes_input(std::ranges::__nothrow_input_range 
auto&&);
  
  static_assert(forward_subsumes_input("foo"));
  1 error generated.

Would that be enough?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3486385 , @gulfem wrote:

> In D119544#3486241 , @erichkeane 
> wrote:
>
>> Ah shucks... Thanks for the heads up.  Is there any chance to get you to get 
>> me a pre-processed version of this failure to play with?  I've not had luck 
>> compiling/running libc++ tests in the past.
>
> Sure, I'll try to help. What exactly do you need?
> Is it possible to revert the change while you are working on the fix?

The change has already been reverted (a 2nd time now, see  
https://github.com/llvm/llvm-project/commit/45c07db31cc76802a1a2e41bed1ce9c1b8198181).
  First time was an amd-32-bit build failure.

If you could run the command line listed above with a -E, and find a way to get 
me the output, I think that would be incredibly helpful!

So based on your above:

  /b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp
  --target=x86_64-unknown-linux-gnu -nostdinc++ -I 
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I 
/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 -I 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support -std=c++2b -Werror -Wall 
-Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes 
-Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type 
-Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare 
-Wunused-variable -Wunused-parameter -Wunreachable-code 
-Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER 
-D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety 
-Wuser-defined-warnings  -fsyntax-only -E

Then pipe that to a file (note the -E I added at the end).  You should get a 
file that looks like some slightly-wonky C++ code.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D119544#3486241 , @erichkeane 
wrote:

> Ah shucks... Thanks for the heads up.  Is there any chance to get you to get 
> me a pre-processed version of this failure to play with?  I've not had luck 
> compiling/running libc++ tests in the past.

Sure, I'll try to help. What exactly do you need?
Is it possible to revert the change while you are working on the fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3486227 , @gulfem wrote:

> We started seeing several test failures after this commit:
> https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8815265760499763361/overview
>
> One example is `nothrow_forward_range.compile.pass.cpp`.
>
>   Script:
>   --
>   : 'COMPILED WITH';  /b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ 
> /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp
>   --target=x86_64-unknown-linux-gnu -nostdinc++ -I 
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I 
> /b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 -I 
> /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support -std=c++2b -Werror -Wall 
> -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes 
> -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type 
> -Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare 
> -Wunused-variable -Wunused-parameter -Wunreachable-code 
> -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER 
> -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety 
> -Wuser-defined-warnings  -fsyntax-only
>   --
>   Exit Code: 1
>   
>   Command Output (stdout):
>   --
>   $ ":" "COMPILED WITH"
>   $ "/b/s/w/ir/x/w/staging/llvm_build/./bin/clang++" 
> "/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
>  "--target=x86_64-unknown-linux-gnu" "-nostdinc++" "-I" 
> "/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1" "-I" 
> "/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1" 
> "-I" "/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support" "-std=c++2b" 
> "-Werror" "-Wall" "-Wextra" "-Wshadow" "-Wundef" 
> "-Wno-unused-command-line-argument" "-Wno-attributes" "-Wno-pessimizing-move" 
> "-Wno-c++11-extensions" "-Wno-noexcept-type" "-Wno-atomic-alignment" 
> "-Wno-user-defined-literals" "-Wsign-compare" "-Wunused-variable" 
> "-Wunused-parameter" "-Wunreachable-code" "-Wno-unused-local-typedef" 
> "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-D_LIBCPP_DISABLE_AVAILABILITY" 
> "-fcoroutines-ts" "-Werror=thread-safety" "-Wuser-defined-warnings" 
> "-fsyntax-only"
>   # command stderr:
>   In file included from 
> /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:18:
>   In file included from 
> /b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support/test_range.h:12:
>   In file included from 
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/ranges:278:
>   In file included from 
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/all.h:18:
>   
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37:
>  error: redefinition of 'operator|'
>   friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
> __closure)
>   ^
>   
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/common_view.h:107:17:
>  note: in instantiation of template class 
> 'std::__range_adaptor_closure' requested 
> here
> struct __fn : __range_adaptor_closure<__fn> {
>   ^
>   
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37:
>  note: previous definition is here
>   friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
> __closure)
>   ^
>   
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27:
>  error: redefinition of 'operator|'
>   friend constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2)
> ^
>   
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27:
>  note: previous definition is here
>   
> /b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37:
>  error: redefinition of 'operator|'
>   friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
> __closure)
>   --

Ah shucks... Thanks for the heads up.  Is there any chance to get you to get me 
a pre-processed version of this failure to play with?  I've not had luck 
compiling/running libc++ tests in the past.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We started seeing several test failures after this commit:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8815265760499763361/overview

One example is `nothrow_forward_range.compile.pass.cpp`.

  Script:
  --
  : 'COMPILED WITH';  /b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp
  --target=x86_64-unknown-linux-gnu -nostdinc++ -I 
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I 
/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 -I 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support -std=c++2b -Werror -Wall 
-Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes 
-Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type 
-Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare 
-Wunused-variable -Wunused-parameter -Wunreachable-code 
-Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER 
-D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety 
-Wuser-defined-warnings  -fsyntax-only
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  $ ":" "COMPILED WITH"
  $ "/b/s/w/ir/x/w/staging/llvm_build/./bin/clang++" 
"/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
 "--target=x86_64-unknown-linux-gnu" "-nostdinc++" "-I" 
"/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1" "-I" 
"/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1" "-I" 
"/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support" "-std=c++2b" "-Werror" 
"-Wall" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" 
"-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" 
"-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wno-user-defined-literals" 
"-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" 
"-Wno-unused-local-typedef" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" 
"-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" "-Werror=thread-safety" 
"-Wuser-defined-warnings" "-fsyntax-only"
  # command stderr:
  In file included from 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:18:
  In file included from 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support/test_range.h:12:
  In file included from 
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/ranges:278:
  In file included from 
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/all.h:18:
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: 
error: redefinition of 'operator|'
  friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
__closure)
  ^
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/common_view.h:107:17: 
note: in instantiation of template class 
'std::__range_adaptor_closure' requested 
here
struct __fn : __range_adaptor_closure<__fn> {
  ^
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: 
note: previous definition is here
  friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
__closure)
  ^
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27: 
error: redefinition of 'operator|'
  friend constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2)
^
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27: 
note: previous definition is here
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: 
error: redefinition of 'operator|'
  friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
__closure)
  --


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4b6c2cd647e9: Deferred Concept Instantiation Implementation 
(authored by erichkeane).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -Wno-unused-value -verify
 
 template  requires ((sizeof(Args) == 1), ...)
 // expected-note@-1 {{because '(sizeof(int) == 1) , (sizeof(char) == 1) , (sizeof(int) == 1)' evaluated to false}}
@@ -40,6 +40,20 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 426060.
erichkeane marked 3 inline comments as done.
erichkeane added a comment.

Make all the changes that @ChuanqiXu suggested.  Thank you again so much for 
your help during this!

I intend to commit this my Monday-AM unless someone comments differently.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -Wno-unused-value -verify
 
 template  requires ((sizeof(Args) == 1), ...)
 // expected-note@-1 {{because '(sizeof(int) == 1) , (sizeof(char) == 1) , (sizeof(int) == 1)' evaluated to false}}
@@ -40,6 +40,20 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 11 inline comments as done.
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:70
+assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good expressions?");
+assert(LHS.isUsable() && RHS.isUsable() && "Side not usable?");
+// We should just be able to 'normalize' these to the builtin Binary

ChuanqiXu wrote:
> I feel like the usage of the API could be further simplified.
Heh, the suggestion is inverted slightly (those should be `!isUsable`) which 
caught me for a while :)  Either way, I think it is a good suggestion.



Comment at: clang/lib/Sema/SemaConcept.cpp:432-476
+if (TemplateArgs) {
+  MultiLevelTemplateArgumentList JustTemplArgs(
+  *FD->getTemplateSpecializationArgs());
+  if (addInstantiatedParametersToScope(
+  FD, PrimaryTemplate->getTemplatedDecl(), Scope, JustTemplArgs))
+return true;
+}

ChuanqiXu wrote:
> The suggested change works. I feel it is not identical with the original. Is 
> it correct or do we miss something in the test?
The very top condition is not exactly the same I think, but in a way that I 
don't believe matters now that this function isn't recursive.

Otherwise I think this is a vast improvement, so I'll keep it!



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2173
 Function->setInstantiationOfMemberFunction(D, TSK_ImplicitInstantiation);
+  } else if (!isFriend) {
+// If this is not a function template, and this is not a friend (that is,

ChuanqiXu wrote:
> The check here is not straightforward. Here we want to check if the Function 
> is local declared function. But what we check here is about friendness. I am 
> not 100% sure if it is right and other reader might be confusing too. I would 
> suggest to check it directly. e.g, check if one of its DeclContext is 
> FunctionDecl.
I cannot come up with a better one here.  Really the condition here is "this is 
not one of the above conditions, and is still not a friend function".  A friend 
could ALSO have a `DeclContext` of a `FunctionDecl` as well, so I don't have a 
good idea of what we could do.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Oh, I'm busy with coroutines these days. Thanks for reminding me for this. LGTM 
to me basically. Only comments for style left.




Comment at: clang/lib/Sema/SemaConcept.cpp:70
+assert((!LHS.isInvalid() && !RHS.isInvalid()) && "not good expressions?");
+assert(LHS.isUsable() && RHS.isUsable() && "Side not usable?");
+// We should just be able to 'normalize' these to the builtin Binary

I feel like the usage of the API could be further simplified.



Comment at: clang/lib/Sema/SemaConcept.cpp:338-340
+  // Backfill the 'converted' list with nulls so we can keep the Converted
+  Converted.append(ConstraintExprs.size() - Converted.size(), nullptr);
+  // and unconverted lists in sync. [temp.constr.op] p2





Comment at: clang/lib/Sema/SemaConcept.cpp:365-370
+  for (unsigned Depth = 0; Depth < TemplateArgsList.getNumSubstitutedLevels();
+   ++Depth) {
+for (unsigned Index = 0;
+ Index < TemplateArgsList.getNumSubstitutedArgs(Depth); ++Index)
+  FlattenedArgs.push_back(TemplateArgsList(Depth, Index));
+  }

I think it is a chance to add range style interfaces for 
MultiLevelTemplateArgumentList. So that we could use range-based loop here.



Comment at: clang/lib/Sema/SemaConcept.cpp:432-476
+if (TemplateArgs) {
+  MultiLevelTemplateArgumentList JustTemplArgs(
+  *FD->getTemplateSpecializationArgs());
+  if (addInstantiatedParametersToScope(
+  FD, PrimaryTemplate->getTemplatedDecl(), Scope, JustTemplArgs))
+return true;
+}

The suggested change works. I feel it is not identical with the original. Is it 
correct or do we miss something in the test?



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2173
 Function->setInstantiationOfMemberFunction(D, TSK_ImplicitInstantiation);
+  } else if (!isFriend) {
+// If this is not a function template, and this is not a friend (that is,

The check here is not straightforward. Here we want to check if the Function is 
local declared function. But what we check here is about friendness. I am not 
100% sure if it is right and other reader might be confusing too. I would 
suggest to check it directly. e.g, check if one of its DeclContext is 
FunctionDecl.



Comment at: clang/lib/Sema/TreeTransform.h:13003
   if (Expr *TRC = E->getCallOperator()->getTrailingRequiresClause())
-// FIXME: Concepts: Substitution into requires clause should only happen
-//  when checking satisfaction.
-NewTrailingRequiresClause = getDerived().TransformExpr(TRC);
+NewTrailingRequiresClause = TRC;
 

I think we could eliminate `NewTrailingRequiresClause`



Comment at: clang/test/SemaTemplate/concepts.cpp:275-276
+template 
+constexpr bool constraint = PR54443::is_same::type,
+ int>::value;
+

Better to rename `constraint` to something like `IsInt`


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3477316 , @erichkeane 
wrote:

> Correct the caching behavior to make the FriendFunc example work.
>
> I THINK this is ready to review!  I'd like to do the lambda examples in a 
> followup patch as I believe that is a pre-existing issue, and this patch has 
> gotten large enough.

Ping @ChuanqiXu : This should be ready for you!  Let me know what changes you 
suggest.




Comment at: clang/test/SemaTemplate/concepts.cpp:471
+  // isn't re-evaluating the constraint?
+  FriendFunc(CFC, 1.0);
+

erichkeane wrote:
> THIS one looks like a regression, it seems to matter which order these two 
> functions are called in, whether it catches it or not.
> 
> So this is the 1 example I have: https://godbolt.org/z/vY4f3961s  Note that 
> if you swap lines 32 and 33, this patch ALSO fails, but in the version in the 
> example, it does NOT fail despite the fact that it SHOULD.
This one was actually pretty easy, I never updated the 'caching' for 
constraints, so we were only doing it based on the outermost level of template 
args, so the two were considered to be the 'same' instantiation for the 
purposes of caching..


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 425516.
erichkeane added a comment.

Correct the caching behavior to make the FriendFunc example work.

I THINK this is ready to review!  I'd like to do the lambda examples in a 
followup patch as I believe that is a pre-existing issue, and this patch has 
gotten large enough.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -Wno-unused-value -verify
 
 template  requires ((sizeof(Args) == 1), ...)
 // expected-note@-1 {{because '(sizeof(int) == 1) , (sizeof(char) == 1) , (sizeof(int) == 1)' evaluated to false}}
@@ -40,6 +40,20 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

OK, 1 issue left (see `FriendFunc`).




Comment at: clang/test/SemaTemplate/concepts.cpp:436
+  SingleDepthReferencesTopLambda(v);
+  // TODO: This should error on constraint failure! (Lambda!)
+  SingleDepthReferencesTopLambda(will_fail);

So see the 'TODO' for all the interesting cases.  Lambdas apparently have not 
had their constraints checked even before this: https://godbolt.org/z/sxjTx7WGn

So I think that is a bug that is 'existing', and I would love to make that a 
separate patch/bug fix.



Comment at: clang/test/SemaTemplate/concepts.cpp:471
+  // isn't re-evaluating the constraint?
+  FriendFunc(CFC, 1.0);
+

THIS one looks like a regression, it seems to matter which order these two 
functions are called in, whether it catches it or not.

So this is the 1 example I have: https://godbolt.org/z/vY4f3961s  Note that if 
you swap lines 32 and 33, this patch ALSO fails, but in the version in the 
example, it does NOT fail despite the fact that it SHOULD.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 425275.
erichkeane added a comment.

Update the concepts-test, remove some extra commented out code.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -Wno-unused-value -verify
 
 template  requires ((sizeof(Args) == 1), ...)
 // expected-note@-1 {{because '(sizeof(int) == 1) , (sizeof(char) == 1) , (sizeof(int) == 1)' evaluated to false}}
@@ -40,6 +40,20 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+3 {{because substituted constraint expression is ill-formed: type 'int' cannot be 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:507
+
+  ContextRAII SavedContext{*this, const_cast(FD)};
+  LocalInstantiationScope Scope(*this, true);

erichkeane wrote:
> This line was the one that fixed the variable lookup in non-templates.
By changing `FD` to `FD->GetNonClosureContext` (with the appropriate casts), I 
was at least able to fix the crash.  The constraint not being checked for 
failure however, is still a problem.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:507
+
+  ContextRAII SavedContext{*this, const_cast(FD)};
+  LocalInstantiationScope Scope(*this, true);

This line was the one that fixed the variable lookup in non-templates.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 425224.
erichkeane added a comment.

Now crashes a few times in concepts.cpp, but sets up the instantiation-scope 
for functions such that local variable references work for them.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -Wno-unused-value -verify
 
 template  requires ((sizeof(Args) == 1), ...)
 // expected-note@-1 {{because '(sizeof(int) == 1) , (sizeof(char) == 1) , (sizeof(int) == 1)' evaluated to false}}
@@ -40,6 +40,20 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+3 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I found




Comment at: clang/test/SemaTemplate/concepts.cpp:386-388
+  []()
+requires(constraint)
+  {}();

erichkeane wrote:
> ChuanqiXu wrote:
> > We might need more negative tests.
> > Now it would pass even if I write:
> > ```
> > []()
> > requires(false)
> > {}();
> > ```
> Ouch!  I'll have to see if I can find where we're missing that check!
Well, I fixed most of the OTHER stuff I think by setting up the context 
properly (see incoming patch!), but this ends up failing crashing, as does a 
few other things in this test.  Still looking through it, but want to get you 
the latest.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2431-2439
+  //if (TrailingRequiresClause) {
+  //  Sema::CXXThisScopeRAII ThisScope(SemaRef, Record, 
D->getMethodQualifiers());
+  //  ExprResult Rebuilt =
+  //  SemaRef.RebuildExprInCurrentInstantiation(TrailingRequiresClause);
+  //  if (Rebuilt.isInvalid())
+  //return nullptr;
+  //  TrailingRequiresClause = Rebuilt.get();

ChuanqiXu wrote:
> The failing test case could be fixed by the above change. The change above is 
> just a hack instead of a suggestion.
> The rationale is that we need to record some information when we traverse the 
> template decl. The bugs now looks like we forget to do something which would 
> be done at the first time. And we meet in problems due to the missed 
> information (we couldn't find the member in the class).
So this would un-do the 'deferred constraint checking', because that woudl 
cause us to immediately instantiate the template, so I don't think we want to 
do that.



Comment at: clang/test/SemaTemplate/concepts.cpp:386-388
+  []()
+requires(constraint)
+  {}();

ChuanqiXu wrote:
> We might need more negative tests.
> Now it would pass even if I write:
> ```
> []()
> requires(false)
> {}();
> ```
Ouch!  I'll have to see if I can find where we're missing that check!


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D119544#3472537 , @erichkeane 
wrote:

> Alright, a touch more:  It looks like the looping through the `CurContext` at 
> line 6084 finds the wrong thing in evaluating this `CXXMethodDecl`!  The 
> "broken" version has its CurContext as `bar` in the example above, the 
> "working" versions have `foo` or `foo2`.  Therefore the 2nd time through 
> the loop, those both end up on the `ClassTemplateSpecializationDecl`, but the 
> broken one finds the `isFileContext` version.
>
> So I don't know HOW to fix it, but it seems that the 
> `CheckFunctionConstraints` probably has to change the `CurContext`.  I 
> suspect the instantiation of the `WorkingRequires` does something like this 
> correctly.
>
> I'm poking on/off on it today, but if @ChuanqiXu has an idea/help, it would 
> be greatly appreciated.

Yeah, I also find the information looks in a mess. I spent some time to look at 
the bug (my workaround shows below) but it would make deferred-concept-inst.cpp 
fail... it would say `foo()` is not a member of `Test`... I don't have strong 
proves here. But I guess we might need to look at tree transformation for 
RequireExpr in `TreeTransform::TransformRequiresExpr`.




Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2431-2439
+  //if (TrailingRequiresClause) {
+  //  Sema::CXXThisScopeRAII ThisScope(SemaRef, Record, 
D->getMethodQualifiers());
+  //  ExprResult Rebuilt =
+  //  SemaRef.RebuildExprInCurrentInstantiation(TrailingRequiresClause);
+  //  if (Rebuilt.isInvalid())
+  //return nullptr;
+  //  TrailingRequiresClause = Rebuilt.get();

The failing test case could be fixed by the above change. The change above is 
just a hack instead of a suggestion.
The rationale is that we need to record some information when we traverse the 
template decl. The bugs now looks like we forget to do something which would be 
done at the first time. And we meet in problems due to the missed information 
(we couldn't find the member in the class).



Comment at: clang/test/SemaTemplate/concepts.cpp:386-388
+  []()
+requires(constraint)
+  {}();

We might need more negative tests.
Now it would pass even if I write:
```
[]()
requires(false)
{}();
```


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Alright, a touch more:  It looks like the looping through the `CurContext` at 
line 6084 finds the wrong thing in evaluating this `CXXMethodDecl`!  The 
"broken" version has its CurContext as `bar` in the example above, the 
"working" versions have `foo` or `foo2`.  Therefore the 2nd time through 
the loop, those both end up on the `ClassTemplateSpecializationDecl`, but the 
broken one finds the `isFileContext` version.

So I don't know HOW to fix it, but it seems that the `CheckFunctionConstraints` 
probably has to change the `CurContext`.  I suspect the instantiation of the 
`WorkingRequires` does something like this correctly.

I'm poking on/off on it today, but if @ChuanqiXu has an idea/help, it would be 
greatly appreciated.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Here is the example I'm playing with:

  template
  constexpr bool constraint = true;
  
  #define BROKE_REQUIRES 1
  #define WORKING_REQUIRES 2
  #define WORKING_BODY 3
   
  #define TEST BROKE_REQUIRES
  //#define TEST WORKING_REQUIRES
  //#define TEST WORKING_BODY
   
  template
  struct S {
T t;
void foo()
  #if TEST == BROKE_REQUIRES
  requires (constraint)
  #endif
  {
  #if TEST == WORKING_BODY
  using G = decltype(t);
  #endif
}
  #if TEST == WORKING_REQUIRES
template
void foo2() requires (constraint){}
  #endif
  };
   
  void bar() {
S s;
  #if TEST == BROKE_REQUIRES || TEST == WORKING_BODY
s.foo();
  #endif
  #if TEST == WORKING_REQUIRES
s.foo2();
  #endif
  }

For all 3 versions (see the definitions of TEST), they get to 6083 on the 1st 
breakpoint.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 424963.
erichkeane added a comment.
This revision is now accepted and ready to land.

Here is my current 'WIP' patch, with only `ChecksMemberVar::foo` failing.  I 
noticed that the problem so far starts in `TransformMemberExpr`, which calls 
`TransformDecl` (TreeTransform.h: ~11018). THAT ends up being a 'pass through' 
and just calls `SemaRef.FindInstantiatedDecl`, which attempts to get the 
instantiated version of `S` in its call to `FindInstantiatedContext` (see 
SemaTemplateInstantiateDecl.cpp:6160).

THAT calls `FindInstantiatedDecl` again on the `ParentDC` (which is the 
uninstantiated `CXXRecordDecl` of `S`), and ends up going down past the 
`ClassTemplate` calculation (~6083, which is DeclContext *DC = CurContext), and 
everything is the same up to there.

That is the point that I have ended looking into it at the moment, but I think 
the problem/or at least something that should suggest the fix is there, since 
it manages to instead return the primary template  instead of the 
instantiation-of-int.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/test/SemaTemplate/concepts.cpp:391
+
+  CausesFriendConstraint CFC;
+  FriendFunc(CFC, 1);

ChuanqiXu wrote:
> erichkeane wrote:
> > erichkeane wrote:
> > > A bunch of the tests below this all fail.
> > See these two tests, which fail by saying that "::local is not a member of 
> > class 'X'".
> I've spent some time to look into this. I don't find the root cause now. But 
> I find that it is related to the wrong lookups. The error is emitted in 
> CheckQualifiedMemberReference. And it looks like we lookup for the original S 
> instead of the instantiated S. And the compiler thinks the 2 structs are 
> different. So here is the error. Do you have any other ideas?
I unfortunately had little time to work on this over the weekend, but right 
before I left on Friday realized that I think the idea of doing 
`RebuildExprInCurrentInstantiation` was the incorrect one.  It ATTEMPTS to 
rebuild things, but doesn't have enough information to do so (which is the case 
you're seeing I think?).  If I change that back to what I had before in every 
case, all except `CheckMemberVar::foo` fails (note `foo2` does not!).  

THAT I have tracked down to `TemplateInstantiator::TransformDecl` failing to 
actually transform the variable from `S` to `S` as you noticed.

It gets me to 3 cases where that function is called, 2 of which work.  
1- `CheckMemberVar::foo`: Fails
2- `CheckMemberVar::foo2`: Works
3- Similar example with the instantiation in the BODY of the func: Works

I am going to spend more time trying to look into that hopefully early this 
week.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/SemaTemplate/concepts.cpp:391
+
+  CausesFriendConstraint CFC;
+  FriendFunc(CFC, 1);

erichkeane wrote:
> erichkeane wrote:
> > A bunch of the tests below this all fail.
> See these two tests, which fail by saying that "::local is not a member of 
> class 'X'".
I've spent some time to look into this. I don't find the root cause now. But I 
find that it is related to the wrong lookups. The error is emitted in 
CheckQualifiedMemberReference. And it looks like we lookup for the original S 
instead of the instantiated S. And the compiler thinks the 2 structs are 
different. So here is the error. Do you have any other ideas?


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane planned changes to this revision.
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2329
+  // constraint, so we should just create a copy of the previous one.
+  // TODO: ERICH: Should this be RebuildExprInCurrentInstantiation here?
+  if (!IsEvaluatingAConstraint()) {

I'm still not sure if anything should be rebuilt here, I suspect the answer is 
MAYBE on the named-concept, but it isn't clear to me.



Comment at: clang/test/SemaTemplate/concepts.cpp:391
+
+  CausesFriendConstraint CFC;
+  FriendFunc(CFC, 1);

erichkeane wrote:
> A bunch of the tests below this all fail.
See these two tests, which fail by saying that "::local is not a member of 
class 'X'".


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 424241.
erichkeane added a comment.
This revision is now accepted and ready to land.

Found that the recursive var-decl collection was incorrect, since all the 
values were already in the parent scopes!  So I ended up being able to fix MOST 
of the problems by simply making the 'scope' inherit when instantiating 
constraints.  This should make us use the 'lookup rules' of the parent, which 
is correct I believe.

I have 1 more pair of failures (see commented out tests in concepts.cpp) where 
the lookup for a member-variable doesn't work right.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/cxx2a-constraint-exprs.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -Wno-unused-value -verify
 
 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane planned changes to this revision.
erichkeane added inline comments.



Comment at: clang/test/SemaTemplate/concepts.cpp:391
+
+  CausesFriendConstraint CFC;
+  FriendFunc(CFC, 1);

A bunch of the tests below this all fail.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 424183.
erichkeane added a comment.
This revision is now accepted and ready to land.

Added the tests that still fail to 'concepts.cpp', I still need to figure those 
out.

However, I switched our 'skipping of instantiation' over to use 
`RebuildExprInCurrentInstantiation`, which LOOKS like it is doing the right 
thing in a couple of cases. I thought perhaps this would be useful progress for 
anyone who has ideas/time to poke around to help.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/cxx2a-constraint-exprs.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+  requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};// #FOO
+
+template 
+void TrailingReturn(T)   // #TRAILING
+  requires(sizeof(T) > 2) || // #TRAILING_REQ
+  T::value   // #TRAILING_REQ_VAL
+{};
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -Wno-unused-value -verify
 
 template  requires ((sizeof(Args) == 1), ...)
 // expected-note@-1 {{because '(sizeof(int) == 1) , 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:13002
+  ExprResult NewTrailingRequiresClause =
+  E->getCallOperator()->getTrailingRequiresClause();
 

erichkeane wrote:
> erichkeane wrote:
> > cor3ntin wrote:
> > > That doesn't look right.
> > > At best if you don't transform the trailing return type you wouldn't 
> > > refer to the transformed captures and that is the issue you are seeing 
> > > with
> > > 
> > > ```
> > > // This asserts because 'y' is not in the Scope's "FindInstantiationOf"
> > > [y = x]() requires(constraint){}
> > > ```
> > > 
> > > But i suspect this should explode even more spectacularly in some cases? 
> > > If i understand correctly, it shouldn't be checked - but it still be 
> > > substituted or am i missing something?
> > > 
> > >At best if you don't transform the trailing return type you wouldn't refer 
> > >to the transformed captures and that is the issue you are seeing with
> > 
> > So the point of the patch here is that we cannot transform this until we 
> > need to 'check' this constraint.  I also missed the 'call' of the lambdas 
> > in each of those cases, so it does need to be 'checked', which means it 
> > needs to be 'substituted'.
> > 
> > >But i suspect this should explode even more spectacularly in some cases? 
> > >If i understand correctly, it shouldn't be checked - but it still be 
> > >substituted or am i missing something?
> > 
> > I think it cannot be substituted either, because the type might be 
> > different by the time you get to the need to check, see here: 
> > https://godbolt.org/z/GxP9T6fa1
> Hrm i just discovered "RebuildExprInCurrentInstantiation".  I wonder if 
> all of these places should be doing THAT instead...
>Hrm i just discovered "RebuildExprInCurrentInstantiation". I wonder if all 
>of these places should be doing THAT instead...

This does SOMETHING, just not the right thing :/  It DOES do some work it 
seems, but not enough to update everything?  I have no idea what is messed up 
here.

I'm pretty much out of ideas here.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:13002
+  ExprResult NewTrailingRequiresClause =
+  E->getCallOperator()->getTrailingRequiresClause();
 

erichkeane wrote:
> cor3ntin wrote:
> > That doesn't look right.
> > At best if you don't transform the trailing return type you wouldn't refer 
> > to the transformed captures and that is the issue you are seeing with
> > 
> > ```
> > // This asserts because 'y' is not in the Scope's "FindInstantiationOf"
> > [y = x]() requires(constraint){}
> > ```
> > 
> > But i suspect this should explode even more spectacularly in some cases? 
> > If i understand correctly, it shouldn't be checked - but it still be 
> > substituted or am i missing something?
> > 
> >At best if you don't transform the trailing return type you wouldn't refer 
> >to the transformed captures and that is the issue you are seeing with
> 
> So the point of the patch here is that we cannot transform this until we need 
> to 'check' this constraint.  I also missed the 'call' of the lambdas in each 
> of those cases, so it does need to be 'checked', which means it needs to be 
> 'substituted'.
> 
> >But i suspect this should explode even more spectacularly in some cases? 
> >If i understand correctly, it shouldn't be checked - but it still be 
> >substituted or am i missing something?
> 
> I think it cannot be substituted either, because the type might be different 
> by the time you get to the need to check, see here: 
> https://godbolt.org/z/GxP9T6fa1
Hrm i just discovered "RebuildExprInCurrentInstantiation".  I wonder if all 
of these places should be doing THAT instead...


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:496
+llvm::Optional
+Sema::SetupConstraintCheckingTemplateArgumentsAndScope(
+FunctionDecl *FD, llvm::Optional> TemplateArgs,

erichkeane wrote:
> Ugh... it looks like all of this might just be 'wrong', and I have no idea 
> how to fix it.  @rsmith  ANY advise you could give here would be unbelievably 
> appreciated.
> 
> Basically, anything involving variables besides ParmVarDecls seems to be 
> broken in some way:
> 
> 
> 
> ```
> template
> void foo(T x) {
> 
> // This asserts because 'y' is not in the Scope's "FindInstantiationOf"
> [y = x]() requires(constraint){}
> 
> 
> // This asserts because 'Local' is not in the Scope's "FindInstantiationOf"
> T Local;
> []() requires(constraint){}
> 
> // This gives a "stack nearly exhausted" error, followed by a bunch of "while 
> checking constraint satisfaction for function 'foo' required here'", THEN 
> crashes checking Constraints due to MLTAL being empty at one point (or 
> perhaps just corrupt?). BUT the stack is only 40 deep at the crash.
> struct S {
> int local;
> void foo() requires(constraint){}
> }
> ```
> 
> FURTHER, this is also broken by this patch:
> ```
> template
> struct S {
> T t;
> void foo() requires(constraint){}
> };
> void use() {
> S s;
> s.foo();
> ```
> With "constraints not satisfied" "because substituted constraint expression 
> is ill-formed: 'S::t' is not a member of 'class S'"
> 
> Curiously, it works if 'foo' is itself a template.
> 
> 
> 
> 
> I can't help but think that this attempt to re-generate the instantiation is 
> just BROKEN, and I have no idea how to fix it, or what the right approach is. 
>  BUT I cannot help but think that I'm doing the 'wrong thing'.
> 
Note that each of those lambdas ALSO needs to be called there, I forgot the 
last ().



Comment at: clang/lib/Sema/TreeTransform.h:13002
+  ExprResult NewTrailingRequiresClause =
+  E->getCallOperator()->getTrailingRequiresClause();
 

cor3ntin wrote:
> That doesn't look right.
> At best if you don't transform the trailing return type you wouldn't refer to 
> the transformed captures and that is the issue you are seeing with
> 
> ```
> // This asserts because 'y' is not in the Scope's "FindInstantiationOf"
> [y = x]() requires(constraint){}
> ```
> 
> But i suspect this should explode even more spectacularly in some cases? 
> If i understand correctly, it shouldn't be checked - but it still be 
> substituted or am i missing something?
> 
>At best if you don't transform the trailing return type you wouldn't refer to 
>the transformed captures and that is the issue you are seeing with

So the point of the patch here is that we cannot transform this until we need 
to 'check' this constraint.  I also missed the 'call' of the lambdas in each of 
those cases, so it does need to be 'checked', which means it needs to be 
'substituted'.

>But i suspect this should explode even more spectacularly in some cases? 
>If i understand correctly, it shouldn't be checked - but it still be 
>substituted or am i missing something?

I think it cannot be substituted either, because the type might be different by 
the time you get to the need to check, see here: https://godbolt.org/z/GxP9T6fa1


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:13002
+  ExprResult NewTrailingRequiresClause =
+  E->getCallOperator()->getTrailingRequiresClause();
 

That doesn't look right.
At best if you don't transform the trailing return type you wouldn't refer to 
the transformed captures and that is the issue you are seeing with

```
// This asserts because 'y' is not in the Scope's "FindInstantiationOf"
[y = x]() requires(constraint){}
```

But i suspect this should explode even more spectacularly in some cases? 
If i understand correctly, it shouldn't be checked - but it still be 
substituted or am i missing something?



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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane planned changes to this revision.
erichkeane added a comment.

If anyone can help here, it would be vastly appreciated... I'm simply out of 
ideas on how to make this work.




Comment at: clang/lib/Sema/SemaConcept.cpp:496
+llvm::Optional
+Sema::SetupConstraintCheckingTemplateArgumentsAndScope(
+FunctionDecl *FD, llvm::Optional> TemplateArgs,

Ugh... it looks like all of this might just be 'wrong', and I have no idea how 
to fix it.  @rsmith  ANY advise you could give here would be unbelievably 
appreciated.

Basically, anything involving variables besides ParmVarDecls seems to be broken 
in some way:



```
template
void foo(T x) {

// This asserts because 'y' is not in the Scope's "FindInstantiationOf"
[y = x]() requires(constraint){}


// This asserts because 'Local' is not in the Scope's "FindInstantiationOf"
T Local;
[]() requires(constraint){}

// This gives a "stack nearly exhausted" error, followed by a bunch of "while 
checking constraint satisfaction for function 'foo' required here'", THEN 
crashes checking Constraints due to MLTAL being empty at one point (or perhaps 
just corrupt?). BUT the stack is only 40 deep at the crash.
struct S {
int local;
void foo() requires(constraint){}
}
```

FURTHER, this is also broken by this patch:
```
template
struct S {
T t;
void foo() requires(constraint){}
};
void use() {
S s;
s.foo();
```
With "constraints not satisfied" "because substituted constraint expression is 
ill-formed: 'S::t' is not a member of 'class S'"

Curiously, it works if 'foo' is itself a template.




I can't help but think that this attempt to re-generate the instantiation is 
just BROKEN, and I have no idea how to fix it, or what the right approach is.  
BUT I cannot help but think that I'm doing the 'wrong thing'.



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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Oh no. Let me know if i can help in any way


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Well, crud.  @cor3ntin broke me again :)  I went to rebase to get pre-commit to 
run again, and now the problem is the lambda captures.  Taking a look.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 423901.
erichkeane marked 6 inline comments as done.
erichkeane added a comment.

Thanks for the review @ChuanqiXu !


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};  // #FOO
+
+template 
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || // #TRAILING_REQ
+T::value{};// #TRAILING_REQ_VAL
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -40,6 +40,18 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+2 {{because substituted constraint expression is ill-formed: type 'int' cannot be used prior to '::' because it has no members}}
+  template 
+  static constexpr auto f(U const index) requires(U::foo) { return true; }
+};
+
+// expected-error@+1 {{no matching function for call to 'f'}}
+static_assert(S1::f(1));
+
 constexpr auto value = 0;
 
 template
Index: 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 10 inline comments as done.
erichkeane added inline comments.



Comment at: clang/include/clang/AST/Decl.h:1891
+TK_DependentFunctionTemplateSpecialization,
+// A Dependent function that itself is not a function.
+TK_DependentNonTemplate

ChuanqiXu wrote:
> hmmm, what does this literally mean? In my understanding, it should be:
> 
>A non template function which is in a dependent scope.
> 
> I am just wondering if this is covered by `TK_NonTemplate`.
Woops, I DID mess that up :)  I will switch to basically your wording.

This _IS_ generally covered by TK_NonTemplate (and was before), and I 
considered not adding it at all, but it is really nice to check 
`getTemplatedKind` to see if the value I need is going to be there.



Comment at: clang/include/clang/AST/Decl.h:1942
+  /// For non-templates, this value will be NULL, unless this was instantiated
+  /// as an inner-declared function in another function template, which will
+  /// cause this to have a pointer to a FunctionDecl. For function declarations

ChuanqiXu wrote:
> > an inner-declared function in another function template
> 
> Does this refer to local lambdas or functions in local classes of a template 
> function **only**? If yes, I recommend to reword this. Since I understand it 
> by the review comment instead of the comments itself.
This is actually the cases where it is NOT a local lambda or function object 
inside a function template.  I'll give a reword a go.



Comment at: clang/include/clang/AST/Decl.h:2691-2692
 
+  /// Specify the function that this was instantiated from, despite it not,
+  /// itself being a template.
+  void setInstantiatedFromDecl(FunctionDecl *FD);

ChuanqiXu wrote:
> I can't read the original comment... I am not sure if it is my problem but I 
> think it may be better to reword it.
Yeah, thats a pretty low quality comment as well :/  I'll give it another shot 
that will hopefully be more understandable.  It isn't quite what you said 
though.



Comment at: clang/lib/AST/Decl.cpp:3787
+  assert(TemplateOrSpecialization.isNull() &&
+ "Member function is already a specialization");
+  TemplateOrSpecialization = FD;

ChuanqiXu wrote:
> Do I understand incorrectly? Must it be a member function?
You do not understand incorrectly :/  Copy-paste error.  I'll remove 'member'.



Comment at: clang/lib/AST/DeclBase.cpp:299
+   DC && !DC->isTranslationUnit() && !DC->isNamespace();
+   DC = DC->getParent())
+if (DC->isFunctionOrMethod())

ChuanqiXu wrote:
> From the function name, I image it should be `DC = DC->getLexicalParent`. Is 
> it incorrect?
This is 'exactly' the function above (`getParentFunctionOrMethod`), except it 
uses the `Lexical` context instead of the semantic context.  Would you prefer a 
name of 'getParentFunctionOrMethodLexically`?



Comment at: clang/lib/Sema/SemaConcept.cpp:480-488
+  if (DeclContext *ParentFunc = FD->getParentFunctionOrMethod()) {
+return SetupConstraintScope(cast(ParentFunc), TemplateArgs,
+MLTAL, Scope);
+  } else if (DeclContext *ParentFunc = FD->getLexicalParentFunctionOrMethod()) 
{
+// In the case of functions-declared-in-functions, the DeclContext is the
+// TU, so make sure we get the LEXICAL decl context in this case.
+return SetupConstraintScope(cast(ParentFunc), TemplateArgs,

ChuanqiXu wrote:
> Don't use else-after-return: 
> https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.
> 
> And I am wondering if we could hit these 2 checks only if the FD is 
> TK_DependentNonTemplate. If yes, I think we could move these two checks in 
> the above block. In this way, the code could be simplified further.
Changes made!


>And I am wondering if we could hit these 2 checks only...
We cannot, this applies generally, including in cases where the current 
function is a local lambda or function object.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/AST/Decl.h:1891
+TK_DependentFunctionTemplateSpecialization,
+// A Dependent function that itself is not a function.
+TK_DependentNonTemplate

hmmm, what does this literally mean? In my understanding, it should be:

   A non template function which is in a dependent scope.

I am just wondering if this is covered by `TK_NonTemplate`.



Comment at: clang/include/clang/AST/Decl.h:1942
+  /// For non-templates, this value will be NULL, unless this was instantiated
+  /// as an inner-declared function in another function template, which will
+  /// cause this to have a pointer to a FunctionDecl. For function declarations

> an inner-declared function in another function template

Does this refer to local lambdas or functions in local classes of a template 
function **only**? If yes, I recommend to reword this. Since I understand it by 
the review comment instead of the comments itself.



Comment at: clang/include/clang/AST/Decl.h:2691-2692
 
+  /// Specify the function that this was instantiated from, despite it not,
+  /// itself being a template.
+  void setInstantiatedFromDecl(FunctionDecl *FD);

I can't read the original comment... I am not sure if it is my problem but I 
think it may be better to reword it.



Comment at: clang/include/clang/AST/DeclBase.h:909-910
 
+  /// Does the same thing as getParentFunctionOrMethod, except starts withthe
+  /// Lexical declaration context instead.
+  const DeclContext *getLexicalParentFunctionOrMethod() const;





Comment at: clang/lib/AST/Decl.cpp:3787
+  assert(TemplateOrSpecialization.isNull() &&
+ "Member function is already a specialization");
+  TemplateOrSpecialization = FD;

Do I understand incorrectly? Must it be a member function?



Comment at: clang/lib/AST/DeclBase.cpp:299
+   DC && !DC->isTranslationUnit() && !DC->isNamespace();
+   DC = DC->getParent())
+if (DC->isFunctionOrMethod())

From the function name, I image it should be `DC = DC->getLexicalParent`. Is it 
incorrect?



Comment at: clang/lib/Sema/SemaConcept.cpp:480-488
+  if (DeclContext *ParentFunc = FD->getParentFunctionOrMethod()) {
+return SetupConstraintScope(cast(ParentFunc), TemplateArgs,
+MLTAL, Scope);
+  } else if (DeclContext *ParentFunc = FD->getLexicalParentFunctionOrMethod()) 
{
+// In the case of functions-declared-in-functions, the DeclContext is the
+// TU, so make sure we get the LEXICAL decl context in this case.
+return SetupConstraintScope(cast(ParentFunc), TemplateArgs,

Don't use else-after-return: 
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.

And I am wondering if we could hit these 2 checks only if the FD is 
TK_DependentNonTemplate. If yes, I think we could move these two checks in the 
above block. In this way, the code could be simplified further.



Comment at: clang/lib/Sema/SemaTemplate.cpp:4705
   CheckConstraintSatisfaction(
-  NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+  NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
   SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),

erichkeane wrote:
> ChuanqiXu wrote:
> > erichkeane wrote:
> > > ChuanqiXu wrote:
> > > > I would feel better if we could write:
> > > > ```
> > > > CheckConstraintSatisfaction(
> > > >   NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
> > > >   SourceRange(SS.isSet() ? SS.getBeginLoc() : 
> > > > ConceptNameInfo.getLoc(),
> > > >   TemplateArgs->getRAngleLoc()),
> > > >   Satisfaction)
> > > > ```
> > > > 
> > > > But it looks unimplementable.
> > > I'm not sure I get the suggestion?  Why would you want to put the 
> > > `MultiLevelTemplateArgumentList` in curleys?  
> > I just feel like the style is more cleaner. But I found the constructor 
> > might not allow us to do so... So this one might not be a suggestion.
> Ah, you mean to pass 'converted' directly in, so:
> ```
> CheckConstraintSatisfaction(
>   NamedConcept, {NamedConcept->getConstraintExpr()}, {Converted},
>   SourceRange(SS.isSet() ? SS.getBeginLoc() : 
> ConceptNameInfo.getLoc(),
>   TemplateArgs->getRAngleLoc()),
>   Satisfaction)
> ```
> 
> (notice 'Converted' instead of MLTAL).  I agree with you, that WOULD be 
> nicer, but unfortunately I think the constructor for that type was created 
> explicitly to avoid us doing that :)
Yeah, this is what mean. I understood it is not easy/good to implement.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2163
+  } else if (!isFriend) {
+

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 423713.
erichkeane added a comment.
Herald added a subscriber: martong.
Herald added a reviewer: shafik.

Fixed the issue that Corentin's test came up with, added a number of others.

The problem is that we didn't properly 'collect' the parameters in cases where 
we referred to a parameter of a function that contains the current function, so 
this patch did a bit of refactoring to make sure we did so recursively.

Additionally, we found an additional case (the non-functor case) where we 
didn't have a way to get the primary template, so we added a new entry in the 
FunctionDecl's PointerUnion to handle these cases.

Please review!  I'd love to get this into trunk with time to bake before the 
branch.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};  // #FOO
+
+template 
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || // #TRAILING_REQ
+T::value{};// #TRAILING_REQ_VAL
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -40,6 +40,18 @@
 
 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

FWIW, I am about 95% sure I have a hold on this, plus a bunch of other cases I 
came up with.  I likely won't get a patch up for review today (in the next 
hour!) unless something miraculous happens, but hopefully I'll have something 
tomorrow for folks to take another look at.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3459169 , @tahonermann 
wrote:

>> I wouldn't think so either? In this case the problem is that 'u' is not in 
>> the re-manufactured scope, I think there is a bit of work to make sure that 
>> lambdas ALSO get the scope of their containing function, if they are in a 
>> functiondecl.
>
> I wouldn't expect lambdas to require special handling; I think they should be 
> handled via their transformation to a member function of a dependent local 
> class or dependent member class.

Yeah, its not lambda-specific, thats just the example here.  The example you 
gave offline shows that this is the case with a dependent local class as well.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> I wouldn't think so either? In this case the problem is that 'u' is not in 
> the re-manufactured scope, I think there is a bit of work to make sure that 
> lambdas ALSO get the scope of their containing function, if they are in a 
> functiondecl.

I wouldn't expect lambdas to require special handling; I think they should be 
handled via their transformation to a member function of a dependent local 
class or dependent member class.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3459045 , @tahonermann 
wrote:

>> This is a case where the function is a template instantiation but does NOT 
>> have a primary template, so I have to figure out what THAT means/what I 
>> should be using instead.
>
> I think that is not supposed to be possible. For example, 
> `FunctionDecl::isFunctionTemplateSpecialization()` will return `false` if 
> there is no primary template.

I wouldn't think so either?  In this case the problem is that 'u' is not in the 
re-manufactured scope, I think there is a bit of work to make sure that lambdas 
ALSO get the scope of their containing function, if they are in a functiondecl.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> This is a case where the function is a template instantiation but does NOT 
> have a primary template, so I have to figure out what THAT means/what I 
> should be using instead.

I think that is not supposed to be possible. For example, 
`FunctionDecl::isFunctionTemplateSpecialization()` will return `false` if there 
is no primary template.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D119544#3458966 , @cor3ntin wrote:

> In D119544#3458848 , @erichkeane 
> wrote:
>
>> I went to commit this, and found that a recently lit test now fails with a 
>> crash during constraint instantiation!  I'll be looking into that.  The 
>> example reduces to:
>>
>>   template
>>   constexpr bool constraint = true;
>>
>>   template < typename U>
>>   void dependent(U&& u) {
>> []() requires constraint {}();
>>   }
>>
>>   void test_dependent() {
>> int v   = 0;
>> dependent(v);
>>   }
>
> I suspect this may be related to my recent lambda changes - not 100% certain 
> but I'm looking into it too

I've got it down to the `SetupConstraintCheckingTemplateArgumentsAndScope` that 
I added.  This is a case where the function is a template instantiation but 
does NOT have a primary template, so I have to figure out what THAT means/what 
I should be using instead.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

In D119544#3458848 , @erichkeane 
wrote:

> I went to commit this, and found that a recently lit test now fails with a 
> crash during constraint instantiation!  I'll be looking into that.  The 
> example reduces to:
>
>   template
>   constexpr bool constraint = true;
>
>   template < typename U>
>   void dependent(U&& u) {
> []() requires constraint {}();
>   }
>
>   void test_dependent() {
> int v   = 0;
> dependent(v);
>   }

I suspect this may be related to my recent lambda changes - not 100% certain 
but I'm looking into it too


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I went to commit this, and found that a recently lit test now fails with a 
crash during constraint instantiation!  I'll be looking into that.  The example 
reduces to:

  template
  constexpr bool constraint = true;
   
  template 
  void dependent(U&& u) {
[]() requires constraint {}();
  }
   
  void test_dependent() {
int v   = 0;
dependent(v);
  }




Comment at: clang/lib/Sema/SemaTemplate.cpp:4705
   CheckConstraintSatisfaction(
-  NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+  NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
   SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),

ChuanqiXu wrote:
> erichkeane wrote:
> > ChuanqiXu wrote:
> > > I would feel better if we could write:
> > > ```
> > > CheckConstraintSatisfaction(
> > >   NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
> > >   SourceRange(SS.isSet() ? SS.getBeginLoc() : 
> > > ConceptNameInfo.getLoc(),
> > >   TemplateArgs->getRAngleLoc()),
> > >   Satisfaction)
> > > ```
> > > 
> > > But it looks unimplementable.
> > I'm not sure I get the suggestion?  Why would you want to put the 
> > `MultiLevelTemplateArgumentList` in curleys?  
> I just feel like the style is more cleaner. But I found the constructor might 
> not allow us to do so... So this one might not be a suggestion.
Ah, you mean to pass 'converted' directly in, so:
```
CheckConstraintSatisfaction(
  NamedConcept, {NamedConcept->getConstraintExpr()}, {Converted},
  SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
  TemplateArgs->getRAngleLoc()),
  Satisfaction)
```

(notice 'Converted' instead of MLTAL).  I agree with you, that WOULD be nicer, 
but unfortunately I think the constructor for that type was created explicitly 
to avoid us doing that :)


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 421564.
erichkeane marked an inline comment as done.
erichkeane added a comment.

rebase for CI + comment change requested.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};  // #FOO
+
+template 
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || // #TRAILING_REQ
+T::value{};// #TRAILING_REQ_VAL
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -40,6 +40,18 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+2 {{because substituted constraint expression is ill-formed: type 'int' cannot be used prior to '::' because it has no members}}
+  template 
+  static constexpr auto f(U const index) requires(U::foo) { return true; }
+};
+
+// expected-error@+1 {{no matching function for call to 'f'}}
+static_assert(S1::f(1));
+
 constexpr auto value = 0;
 
 template
Index: clang/test/SemaTemplate/deferred-concept-inst.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/deferred-concept-inst.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify -Wno-unused-value
+// expected-no-diagnostics
+
+namespace GithubBug44178 {

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

LGTM basically. Please wait for 1~2 weeks to land this in case there are other 
comments.




Comment at: clang/lib/Sema/SemaConcept.cpp:496-497
+
+  // Attn Reviewers: we need to do this for the function constraints for
+  // comparison of constraints to work, but do we also need to do it for
+  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we

We need to edit the `Attn Reviewers` to `TODO` before landing. The content need 
to be rewording too. Your English is much better than me. So no concrete 
suggestion here : )



Comment at: clang/lib/Sema/SemaTemplate.cpp:4705
   CheckConstraintSatisfaction(
-  NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+  NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
   SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),

erichkeane wrote:
> ChuanqiXu wrote:
> > I would feel better if we could write:
> > ```
> > CheckConstraintSatisfaction(
> >   NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
> >   SourceRange(SS.isSet() ? SS.getBeginLoc() : 
> > ConceptNameInfo.getLoc(),
> >   TemplateArgs->getRAngleLoc()),
> >   Satisfaction)
> > ```
> > 
> > But it looks unimplementable.
> I'm not sure I get the suggestion?  Why would you want to put the 
> `MultiLevelTemplateArgumentList` in curleys?  
I just feel like the style is more cleaner. But I found the constructor might 
not allow us to do so... So this one might not be a suggestion.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 421186.
erichkeane marked 2 inline comments as done.
erichkeane added a comment.

Make suggestions from @ChuanqiXu


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};  // #FOO
+
+template 
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || // #TRAILING_REQ
+T::value{};// #TRAILING_REQ_VAL
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -40,6 +40,18 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+2 {{because substituted constraint expression is ill-formed: type 'int' cannot be used prior to '::' because it has no members}}
+  template 
+  static constexpr auto f(U const index) requires(U::foo) { return true; }
+};
+
+// expected-error@+1 {{no matching function for call to 'f'}}
+static_assert(S1::f(1));
+
 constexpr auto value = 0;
 
 template
Index: clang/test/SemaTemplate/deferred-concept-inst.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/deferred-concept-inst.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify -Wno-unused-value
+// expected-no-diagnostics
+
+namespace GithubBug44178 {

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 8 inline comments as done.
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:4705
   CheckConstraintSatisfaction(
-  NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+  NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
   SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),

ChuanqiXu wrote:
> I would feel better if we could write:
> ```
> CheckConstraintSatisfaction(
>   NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
>   SourceRange(SS.isSet() ? SS.getBeginLoc() : 
> ConceptNameInfo.getLoc(),
>   TemplateArgs->getRAngleLoc()),
>   Satisfaction)
> ```
> 
> But it looks unimplementable.
I'm not sure I get the suggestion?  Why would you want to put the 
`MultiLevelTemplateArgumentList` in curleys?  



Comment at: clang/lib/Sema/SemaTemplate.cpp:7468-7483
+  // Attn Reviewers: This works and fixes the constraint comparison issues,
+  // but I don't have a good idea why this is, nor if it is the 'right'
+  // thing.  I THINK it is pulling off the 'template template' level of the
+  // constraint, but I have no idea if that is the correct thing to do.
+  SmallVector ConvertedParamsAC;
+  TemplateArgumentList Empty(TemplateArgumentList::OnStack, {});
+  MultiLevelTemplateArgumentList MLTAL{Empty};

ChuanqiXu wrote:
> I've spent some time to playing it myself to figure it out. And I found that 
> we could fix this cleaner we adopt above suggestions. The problem here is 
> that the instantiation is started half way. But the conversion for the 
> constraint have been deferred. So here is the problem. I guess there are 
> other similar problems. But let's fix them after we met them actually.
> 
Ah, neat!  Thanks for this.  Done.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I've read the whole patch. It looks good to me roughly except some style issues 
and the 2 places you marked as ` Attn Reviewers`. Let's try to fix them one by 
one.




Comment at: clang/lib/Sema/SemaTemplate.cpp:4705
   CheckConstraintSatisfaction(
-  NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+  NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
   SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),

I would feel better if we could write:
```
CheckConstraintSatisfaction(
  NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
  SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
  TemplateArgs->getRAngleLoc()),
  Satisfaction)
```

But it looks unimplementable.



Comment at: clang/lib/Sema/SemaTemplate.cpp:5564
   return true;
 
 TemplateArgumentList TemplateArgs(TemplateArgumentList::OnStack, 
Converted);





Comment at: clang/lib/Sema/SemaTemplate.cpp:7468-7483
+  // Attn Reviewers: This works and fixes the constraint comparison issues,
+  // but I don't have a good idea why this is, nor if it is the 'right'
+  // thing.  I THINK it is pulling off the 'template template' level of the
+  // constraint, but I have no idea if that is the correct thing to do.
+  SmallVector ConvertedParamsAC;
+  TemplateArgumentList Empty(TemplateArgumentList::OnStack, {});
+  MultiLevelTemplateArgumentList MLTAL{Empty};

I've spent some time to playing it myself to figure it out. And I found that we 
could fix this cleaner we adopt above suggestions. The problem here is that the 
instantiation is started half way. But the conversion for the constraint have 
been deferred. So here is the problem. I guess there are other similar 
problems. But let's fix them after we met them actually.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:60-61
 const NamedDecl *D, const TemplateArgumentList *Innermost,
-bool RelativeToPrimary, const FunctionDecl *Pattern) {
+bool RelativeToPrimary, const FunctionDecl *Pattern, bool LookBeyondLambda,
+bool IncludeContainingStructArgs) {
   // Accumulate the set of template argument lists in this structure.

erichkeane wrote:
> ChuanqiXu wrote:
> > Would you elaborate more for `LookBeyondLambda` and 
> > `IncludeContainingStructArgs`? It confuses me since I couldn't find 
> > `Lambda` or `Struct` from the context of use point.
> Sure!  
> 
> So this function is typically used for 'getting the template instantiation 
> args' of the current Declaration (D) for a variety of reasons.  In all of 
> those cases previously, it would 'stop' looking when it hit a lambda generic 
> call operator (see line 157).  This would block our ability to get the full 
> instantiation tree.
> 
> Similarly, it would stop at a containing class-template (as most 
> instantiations are done against the parent class template).  Unfortunately 
> this is sufficient, so the IncludeContainingStructArgs (see line 185) ALSO 
> includes those arguments, as they are necessary  (since they haven't been 
> instantiated in the constraint yet).
I got it. It might be necessary to edit the comment too.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:60-61
 const NamedDecl *D, const TemplateArgumentList *Innermost,
-bool RelativeToPrimary, const FunctionDecl *Pattern) {
+bool RelativeToPrimary, const FunctionDecl *Pattern, bool LookBeyondLambda,
+bool IncludeContainingStructArgs) {
   // Accumulate the set of template argument lists in this structure.

ChuanqiXu wrote:
> Would you elaborate more for `LookBeyondLambda` and 
> `IncludeContainingStructArgs`? It confuses me since I couldn't find `Lambda` 
> or `Struct` from the context of use point.
Sure!  

So this function is typically used for 'getting the template instantiation 
args' of the current Declaration (D) for a variety of reasons.  In all of those 
cases previously, it would 'stop' looking when it hit a lambda generic call 
operator (see line 157).  This would block our ability to get the full 
instantiation tree.

Similarly, it would stop at a containing class-template (as most instantiations 
are done against the parent class template).  Unfortunately this is sufficient, 
so the IncludeContainingStructArgs (see line 185) ALSO includes those 
arguments, as they are necessary  (since they haven't been instantiated in the 
constraint yet).


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:489
+  // Attn Reviewers: we need to do this for the function constraints for
+  // comparison of constraints to work, but do we also need to do it for
+  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we

erichkeane wrote:
> ChuanqiXu wrote:
> > erichkeane wrote:
> > > ChuanqiXu wrote:
> > > > Would you mind to elaborate for the issue "function constraints for 
> > > > comparison of constraints to work" more? Maybe it is said in previous 
> > > > messages, but the history is hard to follow...
> > > Yep, this one is difficult :/  Basically, when we instantiate the 
> > > constraints at 'checking' time, if the function is NOT a template, we 
> > > call `CheckFunctionConstraints`. When we go to check the 'subsumption' 
> > > type things with a fully instantiated version, they fail if they are 
> > > dependent (as it is expected that way).  See the 
> > > `setTrailingRequiresClause` here.
> > > 
> > > HOWEVER, it seems we ALWAYS pick constraints off the primary template, 
> > > rather than the 'stored' version of the constraint on the current 
> > > declaration.  I tried to do something similar here for those cases, but 
> > > 1: it had no effect seemingly, and 2: it ended up getting complicated in 
> > > many cases (as figuring out the relationship between the constraints in 
> > > the two nodes was difficult/near impossible).
> > > 
> > > I opted to not do it, and I don't THINK it needs to happen over there, 
> > > but I wanted to point out that I was skipping it in case reviewers had a 
> > > better idea.
> > > 
> > > 
> > Let me ask some questions to get in consensus for the problem:
> > 
> > > Basically, when we instantiate the constraints at 'checking' time, if the 
> > > function is NOT a template, we call CheckFunctionConstraints.
> > 
> > I think the function who contains a trailing require clause should be 
> > template one. Do you want to say independent ? Or do you refer the 
> > following case?
> > 
> > ```C++
> > template struct A {
> >   static void f(int) requires ;
> > };
> > ```
> > 
> > And the related question would be what's the cases that 
> > `CheckFunctionConstraints` would be called and what's the cases that 
> > `CheckinstantiatedFunctionTemplateConstraints` would be called?
> > 
> > > When we go to check the 'subsumption' type things with a fully 
> > > instantiated version, they fail if they are dependent (as it is expected 
> > > that way).
> > 
> > If it is fully instantiated, how could it be dependent?
> > 
> > > HOWEVER, it seems we ALWAYS pick constraints off the primary template, 
> > > rather than the 'stored' version of the constraint on the current 
> > > declaration.
> > 
> > 1. What is `we`? I mean what's the function would pick up the constraints 
> > from the primary template.
> > 2. Does `the stored version of the constraint` means the trailing require 
> > clause of FD? Would it be the original one or `Converted[0]`.
> >>Or do you refer the following case?
> 
> Yeah, thats exactly the case I am talking about.  A case where the function 
> itself isn't a template, but is dependent. 
> 
> >>And the related question would be what's the cases that 
> >>CheckFunctionConstraints would be called and what's the cases that 
> >>CheckinstantiatedFunctionTemplateConstraints would be called?
> 
> The former when either the function is not dependent at all, OR it is 
> dependent-but-fully-instantiated (like in the example you gave).  All other 
> cases (where the template is NOT fully instantiated) calls the other function.
> 
> >>If it is fully instantiated, how could it be dependent?
> 
> By "Fully Instantiated" I mean "everything except the constraints (and 
> technically, some noexcept)", since we are deferring constraint checking 
> until that point. A bunch of the changes in this commit STOP us from 
> instantiating the constraint except when checking, since that is the point of 
> the patch!  SO, the constriant is still 'dependent' (like in your example 
> above).  
> 
> 
> >>What is we? I mean what's the function would pick up the constraints from 
> >>the primary template.
> 
> Royal 'we', or clang. The function is `getAssociatedConstraints`.
> 
> >>Does the stored version of the constraint means the trailing require clause 
> >>of FD? Would it be the original one or Converted[0].
> 
> The 'stored' version (that is, not on the primary template) is the one that 
> we partially instantiated when going through earlier instantiations.  In the 
> case that it was a template, we seem to always ignore these instantiated 
> versions.  IN the case where it is NOT a template, we use that for checking 
> (which is why Converted[0] needs replacing here).
> 
I see roughly. The implementation looks a little bit odd to me.. But I don't 
find better solutions..



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:60-61
 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:489
+  // Attn Reviewers: we need to do this for the function constraints for
+  // comparison of constraints to work, but do we also need to do it for
+  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we

ChuanqiXu wrote:
> erichkeane wrote:
> > ChuanqiXu wrote:
> > > Would you mind to elaborate for the issue "function constraints for 
> > > comparison of constraints to work" more? Maybe it is said in previous 
> > > messages, but the history is hard to follow...
> > Yep, this one is difficult :/  Basically, when we instantiate the 
> > constraints at 'checking' time, if the function is NOT a template, we call 
> > `CheckFunctionConstraints`. When we go to check the 'subsumption' type 
> > things with a fully instantiated version, they fail if they are dependent 
> > (as it is expected that way).  See the `setTrailingRequiresClause` here.
> > 
> > HOWEVER, it seems we ALWAYS pick constraints off the primary template, 
> > rather than the 'stored' version of the constraint on the current 
> > declaration.  I tried to do something similar here for those cases, but 1: 
> > it had no effect seemingly, and 2: it ended up getting complicated in many 
> > cases (as figuring out the relationship between the constraints in the two 
> > nodes was difficult/near impossible).
> > 
> > I opted to not do it, and I don't THINK it needs to happen over there, but 
> > I wanted to point out that I was skipping it in case reviewers had a better 
> > idea.
> > 
> > 
> Let me ask some questions to get in consensus for the problem:
> 
> > Basically, when we instantiate the constraints at 'checking' time, if the 
> > function is NOT a template, we call CheckFunctionConstraints.
> 
> I think the function who contains a trailing require clause should be 
> template one. Do you want to say independent ? Or do you refer the following 
> case?
> 
> ```C++
> template struct A {
>   static void f(int) requires ;
> };
> ```
> 
> And the related question would be what's the cases that 
> `CheckFunctionConstraints` would be called and what's the cases that 
> `CheckinstantiatedFunctionTemplateConstraints` would be called?
> 
> > When we go to check the 'subsumption' type things with a fully instantiated 
> > version, they fail if they are dependent (as it is expected that way).
> 
> If it is fully instantiated, how could it be dependent?
> 
> > HOWEVER, it seems we ALWAYS pick constraints off the primary template, 
> > rather than the 'stored' version of the constraint on the current 
> > declaration.
> 
> 1. What is `we`? I mean what's the function would pick up the constraints 
> from the primary template.
> 2. Does `the stored version of the constraint` means the trailing require 
> clause of FD? Would it be the original one or `Converted[0]`.
>>Or do you refer the following case?

Yeah, thats exactly the case I am talking about.  A case where the function 
itself isn't a template, but is dependent. 

>>And the related question would be what's the cases that 
>>CheckFunctionConstraints would be called and what's the cases that 
>>CheckinstantiatedFunctionTemplateConstraints would be called?

The former when either the function is not dependent at all, OR it is 
dependent-but-fully-instantiated (like in the example you gave).  All other 
cases (where the template is NOT fully instantiated) calls the other function.

>>If it is fully instantiated, how could it be dependent?

By "Fully Instantiated" I mean "everything except the constraints (and 
technically, some noexcept)", since we are deferring constraint checking until 
that point. A bunch of the changes in this commit STOP us from instantiating 
the constraint except when checking, since that is the point of the patch!  SO, 
the constriant is still 'dependent' (like in your example above).  


>>What is we? I mean what's the function would pick up the constraints from the 
>>primary template.

Royal 'we', or clang. The function is `getAssociatedConstraints`.

>>Does the stored version of the constraint means the trailing require clause 
>>of FD? Would it be the original one or Converted[0].

The 'stored' version (that is, not on the primary template) is the one that we 
partially instantiated when going through earlier instantiations.  In the case 
that it was a template, we seem to always ignore these instantiated versions.  
IN the case where it is NOT a template, we use that for checking (which is why 
Converted[0] needs replacing here).



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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 419750.
erichkeane added a comment.

Do a rebase, only conflict was with ReleaseNotes.rst.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};  // #FOO
+
+template 
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || // #TRAILING_REQ
+T::value{};// #TRAILING_REQ_VAL
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -40,6 +40,18 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+2 {{because substituted constraint expression is ill-formed: type 'int' cannot be used prior to '::' because it has no members}}
+  template 
+  static constexpr auto f(U const index) requires(U::foo) { return true; }
+};
+
+// expected-error@+1 {{no matching function for call to 'f'}}
+static_assert(S1::f(1));
+
 constexpr auto value = 0;
 
 template
Index: clang/test/SemaTemplate/deferred-concept-inst.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/deferred-concept-inst.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify -Wno-unused-value
+// expected-no-diagnostics
+
+namespace GithubBug44178 {
+template 
+struct CRTP {
+  

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

BTW, it looks like the patch needs to rebase with main so that other people 
could play it self if interested.




Comment at: clang/lib/Sema/SemaConcept.cpp:489
+  // Attn Reviewers: we need to do this for the function constraints for
+  // comparison of constraints to work, but do we also need to do it for
+  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we

erichkeane wrote:
> ChuanqiXu wrote:
> > Would you mind to elaborate for the issue "function constraints for 
> > comparison of constraints to work" more? Maybe it is said in previous 
> > messages, but the history is hard to follow...
> Yep, this one is difficult :/  Basically, when we instantiate the constraints 
> at 'checking' time, if the function is NOT a template, we call 
> `CheckFunctionConstraints`. When we go to check the 'subsumption' type things 
> with a fully instantiated version, they fail if they are dependent (as it is 
> expected that way).  See the `setTrailingRequiresClause` here.
> 
> HOWEVER, it seems we ALWAYS pick constraints off the primary template, rather 
> than the 'stored' version of the constraint on the current declaration.  I 
> tried to do something similar here for those cases, but 1: it had no effect 
> seemingly, and 2: it ended up getting complicated in many cases (as figuring 
> out the relationship between the constraints in the two nodes was 
> difficult/near impossible).
> 
> I opted to not do it, and I don't THINK it needs to happen over there, but I 
> wanted to point out that I was skipping it in case reviewers had a better 
> idea.
> 
> 
Let me ask some questions to get in consensus for the problem:

> Basically, when we instantiate the constraints at 'checking' time, if the 
> function is NOT a template, we call CheckFunctionConstraints.

I think the function who contains a trailing require clause should be template 
one. Do you want to say independent ? Or do you refer the following case?

```C++
template struct A {
  static void f(int) requires ;
};
```

And the related question would be what's the cases that 
`CheckFunctionConstraints` would be called and what's the cases that 
`CheckinstantiatedFunctionTemplateConstraints` would be called?

> When we go to check the 'subsumption' type things with a fully instantiated 
> version, they fail if they are dependent (as it is expected that way).

If it is fully instantiated, how could it be dependent?

> HOWEVER, it seems we ALWAYS pick constraints off the primary template, rather 
> than the 'stored' version of the constraint on the current declaration.

1. What is `we`? I mean what's the function would pick up the constraints from 
the primary template.
2. Does `the stored version of the constraint` means the trailing require 
clause of FD? Would it be the original one or `Converted[0]`.


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:7011
+
+  bool CheckConstraintSatisfaction(
+  const NamedDecl *Template, ArrayRef ConstraintExprs,

ChuanqiXu wrote:
> I think this one need comment too. What's the difference with the above one?
Thanks! I'll comment on that.  The difference is the 'out' parameter of 
'ConvertedConstraints', but I'll put details in the comment.



Comment at: clang/lib/Sema/SemaConcept.cpp:489
+  // Attn Reviewers: we need to do this for the function constraints for
+  // comparison of constraints to work, but do we also need to do it for
+  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we

ChuanqiXu wrote:
> Would you mind to elaborate for the issue "function constraints for 
> comparison of constraints to work" more? Maybe it is said in previous 
> messages, but the history is hard to follow...
Yep, this one is difficult :/  Basically, when we instantiate the constraints 
at 'checking' time, if the function is NOT a template, we call 
`CheckFunctionConstraints`. When we go to check the 'subsumption' type things 
with a fully instantiated version, they fail if they are dependent (as it is 
expected that way).  See the `setTrailingRequiresClause` here.

HOWEVER, it seems we ALWAYS pick constraints off the primary template, rather 
than the 'stored' version of the constraint on the current declaration.  I 
tried to do something similar here for those cases, but 1: it had no effect 
seemingly, and 2: it ended up getting complicated in many cases (as figuring 
out the relationship between the constraints in the two nodes was 
difficult/near impossible).

I opted to not do it, and I don't THINK it needs to happen over there, but I 
wanted to point out that I was skipping it in case reviewers had a better idea.




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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 419463.
erichkeane marked 5 inline comments as done.
erichkeane added a comment.

Respond to @ChuanqiXu  and fix a few comments he suggested.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};  // #FOO
+
+template 
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || // #TRAILING_REQ
+T::value{};// #TRAILING_REQ_VAL
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -40,6 +40,18 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+2 {{because substituted constraint expression is ill-formed: type 'int' cannot be used prior to '::' because it has no members}}
+  template 
+  static constexpr auto f(U const index) requires(U::foo) { return true; }
+};
+
+// expected-error@+1 {{no matching function for call to 'f'}}
+static_assert(S1::f(1));
+
 constexpr auto value = 0;
 
 template
Index: clang/test/SemaTemplate/deferred-concept-inst.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/deferred-concept-inst.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify -Wno-unused-value
+// expected-no-diagnostics
+

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I am interested in this one. But it is absolutely not easy to understand...




Comment at: clang/include/clang/Sema/Sema.h:6991-6993
   /// \param TemplateArgs the list of template arguments to substitute into the
-  /// constraint expression.
+  /// constraint expression, which is multi-level since we have to substitute
+  /// ALL levels for the purposes of checking.

Now the comment is not precise.



Comment at: clang/include/clang/Sema/Sema.h:7011
+
+  bool CheckConstraintSatisfaction(
+  const NamedDecl *Template, ArrayRef ConstraintExprs,

I think this one need comment too. What's the difference with the above one?



Comment at: clang/lib/Sema/SemaConcept.cpp:167
+  return LHSRes.isUsable() ? BO.recreateBinOp(S, LHSRes) : ExprEmpty();
+// return BO.recreateBinOp(S, LHSRes);
 

We should delete this one.



Comment at: clang/lib/Sema/SemaConcept.cpp:177
+  return LHSRes.isUsable() ? BO.recreateBinOp(S, LHSRes) : ExprEmpty();
+// return BO.recreateBinOp(S, LHSRes);
 

ditto



Comment at: clang/lib/Sema/SemaConcept.cpp:429-435
+if (TemplateArgs) {
+  MultiLevelTemplateArgumentList JustTemplArgs(
+  *FD->getTemplateSpecializationArgs());
+  if (addInstantiatedParametersToScope(
+  FD, PrimaryTemplate->getTemplatedDecl(), Scope, JustTemplArgs))
+return {};
+}

Might you elaborate more on this. I am not sure about the intention.



Comment at: clang/lib/Sema/SemaConcept.cpp:489
+  // Attn Reviewers: we need to do this for the function constraints for
+  // comparison of constraints to work, but do we also need to do it for
+  // CheckInstantiatedFunctionConstraints?  That one is more difficult, but we

Would you mind to elaborate for the issue "function constraints for comparison 
of constraints to work" more? Maybe it is said in previous messages, but the 
history is hard to follow...


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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 418235.
erichkeane added a comment.

Apply clang-format


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){};  // #FOO
+
+template 
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || // #TRAILING_REQ
+T::value{};// #TRAILING_REQ_VAL
+template 
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template 
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ_VAL{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -40,6 +40,18 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template 
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+2 {{because substituted constraint expression is ill-formed: type 'int' cannot be used prior to '::' because it has no members}}
+  template 
+  static constexpr auto f(U const index) requires(U::foo) { return true; }
+};
+
+// expected-error@+1 {{no matching function for call to 'f'}}
+static_assert(S1::f(1));
+
 constexpr auto value = 0;
 
 template
Index: clang/test/SemaTemplate/deferred-concept-inst.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/deferred-concept-inst.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -std=c++2a -x c++ %s -verify -Wno-unused-value
+// expected-no-diagnostics
+
+namespace GithubBug44178 {
+template 
+struct CRTP {
+  void call_foo() requires
+  

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 417948.
erichkeane added reviewers: clang-language-wg, aaron.ballman, saar.raz.
erichkeane added a subscriber: cfe-commits.
erichkeane added a comment.

Added release notes.

I tossed every ranges-based example I could find on google at this and I saw 
only improvements (that is, no regressions).  I don't think there is anything 
else I can do on this, so hoping to get this reviewed and committed, hopefully 
soon enough for folks to mess with this enough for more-clever folks to break 
it.


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

https://reviews.llvm.org/D119544

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  clang/test/SemaTemplate/deferred-concept-inst.cpp
  clang/test/SemaTemplate/instantiate-requires-clause.cpp
  clang/test/SemaTemplate/trailing-return-short-circuit.cpp

Index: clang/test/SemaTemplate/trailing-return-short-circuit.cpp
===
--- /dev/null
+++ clang/test/SemaTemplate/trailing-return-short-circuit.cpp
@@ -0,0 +1,61 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template 
+requires(sizeof(T) > 2) || T::value // #FOO_REQ
+void Foo(T){}; // #FOO
+
+template 
+void TrailingReturn(T) // #TRAILING
+requires(sizeof(T) > 2) || T::value // #TRAILING_REQ
+{};
+template
+struct HasValue {
+  static constexpr bool value = B;
+};
+static_assert(sizeof(HasValue) <= 2);
+
+template
+struct HasValueLarge {
+  static constexpr bool value = B;
+  int I;
+};
+static_assert(sizeof(HasValueLarge) > 2);
+
+void usage() {
+  // Passes the 1st check, short-circuit so the 2nd ::value is not evaluated.
+  Foo(1.0);
+  TrailingReturn(1.0);
+
+  // Fails the 1st check, but has a ::value, so the check happens correctly.
+  Foo(HasValue{});
+  TrailingReturn(HasValue{});
+
+  // Passes the 1st check, but would have passed the 2nd one.
+  Foo(HasValueLarge{});
+  TrailingReturn(HasValueLarge{});
+
+  // Fails the 1st check, fails 2nd because there is no ::value.
+  Foo(true);
+  // expected-error@-1{{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  TrailingReturn(true);
+  // expected-error@-1{{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = bool]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(_Bool) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ{{because substituted constraint expression is ill-formed: type 'bool' cannot be used prior to '::' because it has no members}}
+
+  // Fails the 1st check, fails 2nd because ::value is false.
+  Foo(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'Foo'}}
+  // expected-note@#FOO{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#FOO_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#FOO_REQ{{and 'HasValue::value' evaluated to false}}
+  TrailingReturn(HasValue{});
+  // expected-error@-1 {{no matching function for call to 'TrailingReturn'}}
+  // expected-note@#TRAILING{{candidate template ignored: constraints not satisfied [with T = HasValue]}}
+  // expected-note@#TRAILING_REQ{{because 'sizeof(HasValue) > 2' (1 > 2) evaluated to false}}
+  // expected-note@#TRAILING_REQ{{and 'HasValue::value' evaluated to false}}
+}
Index: clang/test/SemaTemplate/instantiate-requires-clause.cpp
===
--- clang/test/SemaTemplate/instantiate-requires-clause.cpp
+++ clang/test/SemaTemplate/instantiate-requires-clause.cpp
@@ -40,6 +40,18 @@
 
 static_assert(S::f(1));
 
+// Similar to the 'S' test, but tries to use 'U' in the requires clause.
+template
+struct S1 {
+  // expected-note@+3 {{candidate template ignored: constraints not satisfied [with U = int]}}
+  // expected-note@+2 {{because substituted constraint expression is ill-formed: type 'int' cannot be used prior to '::' because it has no members}}
+  template 
+  static constexpr auto f(U const index) requires(U::foo) { return true; }
+};
+
+// expected-error@+1 {{no matching function for call to 'f'}}
+static_assert(S1::f(1));
+
 constexpr auto value = 0;
 
 template
Index: