[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:513-534
+  llvm::Optional MLTAL =
+  SetupConstraintCheckingTemplateArgumentsAndScope(
+  const_cast(FD), {}, Scope);
+
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast(FD)) {

erichkeane wrote:
> erichkeane wrote:
> > mizvekov wrote:
> > > mizvekov wrote:
> > > > Unchecked access to MLTAL (Optional).
> > > > 
> > > > Following reduction reproduces a crash here: `-cc1 -std=c++20 
> > > > -fsyntax-only -ferror-limit 19`.
> > > > ```
> > > > template a b < ;
> > > > template c e ag < ;
> > > > ah) ;
> > > > am = ;
> > > > template  class aq {
> > > >   aq(ap...; __attribute__) auto aj() requires(am)
> > > > }
> > > > f() {  [;  aq d;  d.aj
> > > > ```
> > > By the way, disregard that repro, it needs another patch that is not in 
> > > master.
> > > 
> > > The unchecked access still does look suspicious though.
> > Thanks for the comment!  I'll look into it.
> Huh... that must be leftover from some other attempt I made at that, the 
> MLTAL is always set.  I'll do an NFC patch to remove it, but thanks for 
> looking!
Woops... nvm... you're right, in an error condition we can get an errored one.  
I'll add a check.  Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:513-534
+  llvm::Optional MLTAL =
+  SetupConstraintCheckingTemplateArgumentsAndScope(
+  const_cast(FD), {}, Scope);
+
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast(FD)) {

erichkeane wrote:
> mizvekov wrote:
> > mizvekov wrote:
> > > Unchecked access to MLTAL (Optional).
> > > 
> > > Following reduction reproduces a crash here: `-cc1 -std=c++20 
> > > -fsyntax-only -ferror-limit 19`.
> > > ```
> > > template a b < ;
> > > template c e ag < ;
> > > ah) ;
> > > am = ;
> > > template  class aq {
> > >   aq(ap...; __attribute__) auto aj() requires(am)
> > > }
> > > f() {  [;  aq d;  d.aj
> > > ```
> > By the way, disregard that repro, it needs another patch that is not in 
> > master.
> > 
> > The unchecked access still does look suspicious though.
> Thanks for the comment!  I'll look into it.
Huh... that must be leftover from some other attempt I made at that, the MLTAL 
is always set.  I'll do an NFC patch to remove it, but thanks for looking!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:513-534
+  llvm::Optional MLTAL =
+  SetupConstraintCheckingTemplateArgumentsAndScope(
+  const_cast(FD), {}, Scope);
+
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast(FD)) {

mizvekov wrote:
> mizvekov wrote:
> > Unchecked access to MLTAL (Optional).
> > 
> > Following reduction reproduces a crash here: `-cc1 -std=c++20 -fsyntax-only 
> > -ferror-limit 19`.
> > ```
> > template a b < ;
> > template c e ag < ;
> > ah) ;
> > am = ;
> > template  class aq {
> >   aq(ap...; __attribute__) auto aj() requires(am)
> > }
> > f() {  [;  aq d;  d.aj
> > ```
> By the way, disregard that repro, it needs another patch that is not in 
> master.
> 
> The unchecked access still does look suspicious though.
Thanks for the comment!  I'll look into it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:513-534
+  llvm::Optional MLTAL =
+  SetupConstraintCheckingTemplateArgumentsAndScope(
+  const_cast(FD), {}, Scope);
+
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast(FD)) {

mizvekov wrote:
> Unchecked access to MLTAL (Optional).
> 
> Following reduction reproduces a crash here: `-cc1 -std=c++20 -fsyntax-only 
> -ferror-limit 19`.
> ```
> template a b < ;
> template c e ag < ;
> ah) ;
> am = ;
> template  class aq {
>   aq(ap...; __attribute__) auto aj() requires(am)
> }
> f() {  [;  aq d;  d.aj
> ```
By the way, disregard that repro, it needs another patch that is not in master.

The unchecked access still does look suspicious though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:513-534
+  llvm::Optional MLTAL =
+  SetupConstraintCheckingTemplateArgumentsAndScope(
+  const_cast(FD), {}, Scope);
+
   Qualifiers ThisQuals;
   CXXRecordDecl *Record = nullptr;
   if (auto *Method = dyn_cast(FD)) {

Unchecked access to MLTAL (Optional).

Following reduction reproduces a crash here: `-cc1 -std=c++20 -fsyntax-only 
-ferror-limit 19`.
```
template a b < ;
template c e ag < ;
ah) ;
am = ;
template  class aq {
  aq(ap...; __attribute__) auto aj() requires(am)
}
f() {  [;  aq d;  d.aj
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Daisy's bug ended up being more complicated than I expected... there is a lot 
to unpack here.  In the meantime, I've captured the bug here: 
https://github.com/llvm/llvm-project/issues/58368 and will continue looking at 
it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#382 , @erichkeane 
wrote:

> In D126907#3854983 , @cor3ntin 
> wrote:
>
>> @erichkeane I was looking at Daisy meta programming shenanigans and found 
>> this crashes https://cute.godbolt.org/z/6fWoEheKc - seems related to your 
>> work, probably? It used to work in Clang 15. I thought you might want to 
>> know!
>
> Thanks!  I'll take a look.  That DOES look like some other failures I've had 
> with this patch.

I've reduced it a bit to remove the STL stuff, but it looks like the 
template-lambda is a part of it: https://cute.godbolt.org/z/Prh6xsesz

It manages to hit quite a few things that I suspect the 
`getTemplateInstantiationArgs` doesn't get right, so I'm guessing the 'fix' is 
going to be in there somewhere.  I'll start taking a look this week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3854983 , @cor3ntin wrote:

> @erichkeane I was looking at Daisy meta programming shenanigans and found 
> this crashes https://cute.godbolt.org/z/6fWoEheKc - seems related to your 
> work, probably? It used to work in Clang 15. I thought you might want to know!

Thanks!  I'll take a look.  That DOES look like some other failures I've had 
with this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

@erichkeane I was looking at Daisy meta programming shenanigans and found this 
crashes https://cute.godbolt.org/z/6fWoEheKc - seems related to your work, 
probably? It used to work in Clang 15. I thought you might want to know!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a subscriber: Uthkarsh.
erichkeane added a comment.

In D126907#3852975 , @BertalanD wrote:

> (I know this is a very contrived example with `void operator!=`, but that is 
> what CVise spat out, and the actual failures are related to comparison 
> operators too)
>
> edit: hah, it repros when it returns `bool` too -- not sure how that `void` 
> came to be...

Looking more closely at that, I don't think it has anything to do with this 
patch.  I know `operator==` (and the materialized inverse) stuff was worked on 
separately recently by @Uthkarsh in 38b9d313e6945804fffc654f849cfa05ba2c713d 
.  See: 
https://reviews.llvm.org/D134529

The Equality-operator stuff has been particularly awkward in the C++ committee 
I think (whether or not reversed candidates work or not have had a bunch of 
tweaking), so you'll have to follow that up there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

(I know this is a very contrived example with `void operator!=`, but that is 
what CVise spat out, and the actual failures are related to comparison 
operators too)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3852909 , @BertalanD wrote:

> I tried out D135772 , and our build got 
> significantly farther than before! I unfortunately discovered another piece 
> of code that pre babdef27c503c0bbbcc017e9f88affddda90ea4e 
>  Clang 
> and GCC accept in C++20 mode, but Clang trunk does not 
> (https://godbolt.org/z/q1q4nfobK):
>
>   struct String {
> String(char *);
> bool operator==(String const &);
> void operator!=(String const &);
>   };
>   
>   extern char* c;
>   extern String s;
>   int test() {
> return c == s;
>   }
>
>
>
>   :10:12: error: invalid operands to binary expression ('char *' and 
> 'String')
> return c == s;
>~ ^  ~

Thanks!  I'll look into that as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

I tried out D135772 , and our build got 
significantly farther than before! I unfortunately discovered another piece of 
code that pre babdef27c503c0bbbcc017e9f88affddda90ea4e 
 Clang and 
GCC accept in C++20 mode, but Clang trunk does not 
(https://godbolt.org/z/q1q4nfobK):

  struct String {
String(char *);
bool operator==(String const &);
void operator!=(String const &);
  };
  
  extern char* c;
  extern String s;
  int test() {
return c == s;
  }



  :10:12: error: invalid operands to binary expression ('char *' and 
'String')
return c == s;
   ~ ^  ~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3849583 , @erichkeane 
wrote:

> In D126907#3846591 , @erichkeane 
> wrote:
>
>> In D126907#3844788 , @BertalanD 
>> wrote:
>>
>>> Hi @erichkeane,
>>>
>>> This change broke compilation of this program 
>>> (https://godbolt.org/z/KrWGvcf8h; reduced from 
>>> https://github.com/SerenityOS/ladybird):
>>>
>>>   template
>>>   constexpr bool IsSame = false;
>>>   
>>>   template
>>>   constexpr bool IsSame = true;
>>>   
>>>   template
>>>   struct Foo {
>>>   template
>>>   Foo(U&&) requires (!IsSame);
>>>   };
>>>   
>>>   template<>
>>>   struct Foo : Foo {
>>>   using Foo::Foo;
>>>   };
>>>   
>>>   Foo test() { return 0; }
>>>
>>>
>>>
>>>   :18:27: error: invalid reference to function 'Foo': constraints 
>>> not satisfied
>>>   Foo test() { return 0; }
>>> ^
>>>   :10:24: note: because substituted constraint expression is 
>>> ill-formed: value of type '' is not contextually 
>>> convertible to 'bool'
>>>   Foo(U&&) requires (!IsSame);
>>>  ^
>>
>> Thanks for the report!  I'll look into it ASAP.
>
> Quick note: I believe I understand the cause of this, which requires a bit 
> more work than I otherwise would have expected.  I have a candidate patch I'm 
> running through my testing right now that should fix this, but it still needs 
> cleaning up.  Expect it in the next day or two if all goes well.



In D126907#3850735 , @BertalanD wrote:

> In D126907#3849583 , @erichkeane 
> wrote:
>
>> In D126907#3846591 , @erichkeane 
>> wrote:
>>
>>> In D126907#3844788 , @BertalanD 
>>> wrote:
>>>
 Hi @erichkeane,

 This change broke compilation of this program 
 (https://godbolt.org/z/KrWGvcf8h; reduced from 
 https://github.com/SerenityOS/ladybird):

   template
   constexpr bool IsSame = false;
   
   template
   constexpr bool IsSame = true;
   
   template
   struct Foo {
   template
   Foo(U&&) requires (!IsSame);
   };
   
   template<>
   struct Foo : Foo {
   using Foo::Foo;
   };
   
   Foo test() { return 0; }



   :18:27: error: invalid reference to function 'Foo': constraints 
 not satisfied
   Foo test() { return 0; }
 ^
   :10:24: note: because substituted constraint expression is 
 ill-formed: value of type '' is not contextually 
 convertible to 'bool'
   Foo(U&&) requires (!IsSame);
  ^
>>>
>>> Thanks for the report!  I'll look into it ASAP.
>>
>> Quick note: I believe I understand the cause of this, which requires a bit 
>> more work than I otherwise would have expected.  I have a candidate patch 
>> I'm running through my testing right now that should fix this, but it still 
>> needs cleaning up.  Expect it in the next day or two if all goes well.
>
> Thank you for your quick response and for creating this massive yak shave of 
> a patch :D
>
> Please ping me if you want me to test the fix on our code, or if I can help 
> in some other way.

Thanks for the offer!  I ended up taking a less-aggressive yak shave on this 
one, and have a patch here: https://reviews.llvm.org/D135772

If you could give it a try, it would be very useful!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

In D126907#3849583 , @erichkeane 
wrote:

> In D126907#3846591 , @erichkeane 
> wrote:
>
>> In D126907#3844788 , @BertalanD 
>> wrote:
>>
>>> Hi @erichkeane,
>>>
>>> This change broke compilation of this program 
>>> (https://godbolt.org/z/KrWGvcf8h; reduced from 
>>> https://github.com/SerenityOS/ladybird):
>>>
>>>   template
>>>   constexpr bool IsSame = false;
>>>   
>>>   template
>>>   constexpr bool IsSame = true;
>>>   
>>>   template
>>>   struct Foo {
>>>   template
>>>   Foo(U&&) requires (!IsSame);
>>>   };
>>>   
>>>   template<>
>>>   struct Foo : Foo {
>>>   using Foo::Foo;
>>>   };
>>>   
>>>   Foo test() { return 0; }
>>>
>>>
>>>
>>>   :18:27: error: invalid reference to function 'Foo': constraints 
>>> not satisfied
>>>   Foo test() { return 0; }
>>> ^
>>>   :10:24: note: because substituted constraint expression is 
>>> ill-formed: value of type '' is not contextually 
>>> convertible to 'bool'
>>>   Foo(U&&) requires (!IsSame);
>>>  ^
>>
>> Thanks for the report!  I'll look into it ASAP.
>
> Quick note: I believe I understand the cause of this, which requires a bit 
> more work than I otherwise would have expected.  I have a candidate patch I'm 
> running through my testing right now that should fix this, but it still needs 
> cleaning up.  Expect it in the next day or two if all goes well.

Thank you for your quick response and for creating this massive yak shave of a 
patch :D

Please ping me if you want me to test the fix on our code, or if I can help in 
some other way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3846591 , @erichkeane 
wrote:

> In D126907#3844788 , @BertalanD 
> wrote:
>
>> Hi @erichkeane,
>>
>> This change broke compilation of this program 
>> (https://godbolt.org/z/KrWGvcf8h; reduced from 
>> https://github.com/SerenityOS/ladybird):
>>
>>   template
>>   constexpr bool IsSame = false;
>>   
>>   template
>>   constexpr bool IsSame = true;
>>   
>>   template
>>   struct Foo {
>>   template
>>   Foo(U&&) requires (!IsSame);
>>   };
>>   
>>   template<>
>>   struct Foo : Foo {
>>   using Foo::Foo;
>>   };
>>   
>>   Foo test() { return 0; }
>>
>>
>>
>>   :18:27: error: invalid reference to function 'Foo': constraints 
>> not satisfied
>>   Foo test() { return 0; }
>> ^
>>   :10:24: note: because substituted constraint expression is 
>> ill-formed: value of type '' is not contextually convertible 
>> to 'bool'
>>   Foo(U&&) requires (!IsSame);
>>  ^
>
> Thanks for the report!  I'll look into it ASAP.

Quick note: I believe I understand the cause of this, which requires a bit more 
work than I otherwise would have expected.  I have a candidate patch I'm 
running through my testing right now that should fix this, but it still needs 
cleaning up.  Expect it in the next day or two if all goes well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3844788 , @BertalanD wrote:

> Hi @erichkeane,
>
> This change broke compilation of this program 
> (https://godbolt.org/z/KrWGvcf8h; reduced from 
> https://github.com/SerenityOS/ladybird):
>
>   template
>   constexpr bool IsSame = false;
>   
>   template
>   constexpr bool IsSame = true;
>   
>   template
>   struct Foo {
>   template
>   Foo(U&&) requires (!IsSame);
>   };
>   
>   template<>
>   struct Foo : Foo {
>   using Foo::Foo;
>   };
>   
>   Foo test() { return 0; }
>
>
>
>   :18:27: error: invalid reference to function 'Foo': constraints not 
> satisfied
>   Foo test() { return 0; }
> ^
>   :10:24: note: because substituted constraint expression is 
> ill-formed: value of type '' is not contextually convertible 
> to 'bool'
>   Foo(U&&) requires (!IsSame);
>  ^

Thanks for the report!  I'll look into it ASAP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

Hi @erichkeane,

This change broke compilation of this program (https://godbolt.org/z/KrWGvcf8h; 
reduced from https://github.com/SerenityOS/ladybird):

  template
  constexpr bool IsSame = false;
  
  template
  constexpr bool IsSame = true;
  
  template
  struct Foo {
  template
  Foo(U&&) requires (!IsSame);
  };
  
  template<>
  struct Foo : Foo {
  using Foo::Foo;
  };
  
  Foo test() { return 0; }



  :18:27: error: invalid reference to function 'Foo': constraints not 
satisfied
  Foo test() { return 0; }
^
  :10:24: note: because substituted constraint expression is 
ill-formed: value of type '' is not contextually convertible to 
'bool'
  Foo(U&&) requires (!IsSame);
 ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3807707 , @wlei wrote:

> Tested this and confirmed the issue I reported is gone, thanks!

Thank you all for the quick responses!


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Lei Wang via Phabricator via cfe-commits]

wlei added a comment.

Tested this and confirmed the issue I reported is gone, thanks!


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Tested with our internal workloads and things look fine.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3806760 , @Mordante wrote:

> In D126907#3805606 , @erichkeane 
> wrote:
>
>> Now fully runs libcxx modules configuration as well, and passes the 
>> minimization examples from @wlei .
>>
>> @wlei  and anyone else: can you please try running this against your 
>> workloads before I commit it?
>
> I tried this with several configurations (modular, c++2b, c++20, and c++03) 
> in libc++'s bootstrapping build and the tests passed.

Thank you so much!


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

In D126907#3805606 , @erichkeane 
wrote:

> Now fully runs libcxx modules configuration as well, and passes the 
> minimization examples from @wlei .
>
> @wlei  and anyone else: can you please try running this against your 
> workloads before I commit it?

I tried this with several configurations (modular, c++2b, c++20, and c++03) in 
libc++'s bootstrapping build and the tests passed.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Lei Wang via Phabricator via cfe-commits]

wlei added a comment.

In D126907#3805606 , @erichkeane 
wrote:

> Now fully runs libcxx modules configuration as well, and passes the 
> minimization examples from @wlei .
>
> @wlei  and anyone else: can you please try running this against your 
> workloads before I commit it?

Thanks for the fix, I will try to run it internally today.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Failure I noticed was NOT a regression: https://godbolt.org/z/MnvqP88vv


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3805640 , @erichkeane 
wrote:

> I spoke too soon, I found another crash that came out of @wlei s repro from 
> last time.

Actually, what I found was a reduction that had gone haywire before and 
generated invalid code, so I don't think it is worth blocking this patch.  The 
actual preprocessed file from @wlei actually compiles perfectly.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I spoke too soon, I found another crash that came out of @wlei s repro from 
last time.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

I have a replacement patch that should fix all of the bugs I'm aware of.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3777088 , @ldionne wrote:

> In D126907#3746477 , @Mordante 
> wrote:
>
>> Unfortunately there are a lot of different options and combination of 
>> options to test libc++.
>> So it's indeed not possible to test all options with once check-cxx 
>> invocation.
>
> This ^. We could include all the libc++ configurations in a single 
> `check-cxx` invokation, but the tests would run for like 70 hours on most 
> machines. Instead, we test different configurations in different CI jobs.
>
> In D126907#3777056 , @erichkeane 
> wrote:
>
>> Still WIP, but uploading to show that I'm still on this :/
>>
>> The two modules related issues from libcxx are now fixed (as reported by 
>> @Mordante), and that configuration builds and passes all tests with MOST of 
>> this change.
>>
>> HOWEVER, the first of two fixes for @wlei messes up how constraint 
>> expressions on class templates are compared, so the result is ~800 
>> additional libcxx failures.  I'm still working through that.
>
> Just to make sure we're on the same page, I assume most of those issues are 
> things that would have been encountered in user code otherwise and filed as 
> bugs. So in a way, I think libc++ is simply acting like some kind of 
> early-in-the-loop pre-release testing. This is similar to what some other 
> open-source projects like Chrome do -- they build from trunk often and will 
> report actual issues to us early. I do get why it can be annoying and 
> disruptive to landing patches sometimes, though.

Yep, this is the case.  Just frustrating to be surprised by things like this as 
late in the process as I was, so I was a little grumpy above.  Unfortunately it 
seems like each of these cycles costs a month here :/

> Concretely, what we could do is probably add `LLVM_ENABLE_RUNTIMES=libcxx` to 
> the Clang pre-commit CI that already runs. That would catch some (but of 
> course not all) issues. In particular, that should catch issues that would 
> otherwise immediately break the libc++ "Bootstrapping build" CI job. For 
> other issues (like an arbitrary combination of `-std=c++xy -fmodules 
> -fno-rtti`), I think we can only rely on slower feedback when libc++ updates 
> the nightly version of Clang that we use in our CI (which would act like a 
> super-early adopter from the POV of Clang at that point).


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D126907#3746477 , @Mordante wrote:

> Unfortunately there are a lot of different options and combination of options 
> to test libc++.
> So it's indeed not possible to test all options with once check-cxx 
> invocation.

This ^. We could include all the libc++ configurations in a single `check-cxx` 
invokation, but the tests would run for like 70 hours on most machines. 
Instead, we test different configurations in different CI jobs.

In D126907#3777056 , @erichkeane 
wrote:

> Still WIP, but uploading to show that I'm still on this :/
>
> The two modules related issues from libcxx are now fixed (as reported by 
> @Mordante), and that configuration builds and passes all tests with MOST of 
> this change.
>
> HOWEVER, the first of two fixes for @wlei messes up how constraint 
> expressions on class templates are compared, so the result is ~800 additional 
> libcxx failures.  I'm still working through that.

Just to make sure we're on the same page, I assume most of those issues are 
things that would have been encountered in user code otherwise and filed as 
bugs. So in a way, I think libc++ is simply acting like some kind of 
early-in-the-loop pre-release testing. This is similar to what some other 
open-source projects like Chrome do -- they build from trunk often and will 
report actual issues to us early. I do get why it can be annoying and 
disruptive to landing patches sometimes, though.

Concretely, what we could do is probably add `LLVM_ENABLE_RUNTIMES=libcxx` to 
the Clang pre-commit CI that already runs. That would catch some (but of course 
not all) issues. In particular, that should catch issues that would otherwise 
immediately break the libc++ "Bootstrapping build" CI job. For other issues 
(like an arbitrary combination of `-std=c++xy -fmodules -fno-rtti`), I think we 
can only rely on slower feedback when libc++ updates the nightly version of 
Clang that we use in our CI (which would act like a super-early adopter from 
the POV of Clang at that point).


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

In D126907#3740536 , @erichkeane 
wrote:

> For example, this enable-modules flag isn't covered by my local check-cxx, 
> which I went out of my way to run on this patch locally.

Unfortunately there are a lot of different options and combination of options 
to test libc++.
So it's indeed not possible to test all options with once check-cxx invocation.

In D126907#3746408 , @erichkeane 
wrote:

> Fixing the assert was pretty easy, and fixing it revealed the same 8 failures 
> from the buildbot above.

Great to hear you can reproduce the issue!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3740249 , @erichkeane 
wrote:

> FWIW, while I wasn't able to reproduce the problem that @Mordante reported, I 
> found that the test suite as given has a large number of asserts in debug 
> mode while trying to compare parameter-mappings during constraint 
> normalization(assert is `clang-15: 
> /iusers/ekeane1/workspaces/delayed-concepts/clang/lib/AST/ASTContext.cpp:4753:
>  clang::QualType clang::ASTContext::getSubstTemplateTypeParmType(const 
> clang::TemplateTypeParmType*, clang::QualType, llvm::Optional) 
> const: Assertion ```Replacement.isCanonical() && "replacement types must 
> always be canonical"' failed.```).`
>
> One Reproducer is:
>
>   #pragma clang module import std.array
>   template
>   struct iter_value_or_void { using type = void; };
> 
>   template
>   struct iter_value_or_void {
>   using type = std::iter_value_t;
>
> BUT debugging seems to show a lot of 'imported' symbols that I'm not 
> particularly sure how they work, so I suspect they are coming from the 
> clang-module-import.
>
> So I'm going to have to try to figure out what is going on here with the 
> clang modules.

Fixing the assert was pretty easy, and fixing it revealed the same 8 failures 
from the buildbot above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I get that libcxx doesn't run on precommit CI (which is unfortunate for those 
of us modifying the CFE), but it becomes difficult to run locally, when 
check-all (requires check-runtimes or check-cxx) don't cover it, and now even 
check-cxx doesn't cover it.

>> What do you exactly mean with "test all the libcxx things" flag?

For example, this enable-modules flag isn't covered by my local check-cxx, 
which I went out of my way to run on this patch locally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

In D126907#3739923 , @aaron.ballman 
wrote:

> In D126907#3739750 , @Mordante 
> wrote:
>
>> In D126907#3738383 , @erichkeane 
>> wrote:
>>
>>> There was a test I dealt with previously where a ton of the header files 
>>> were run with modules enabled (and an auto generated files), so I'm shocked 
>>> to find there is MORE differences here.  @ldionne : This is another example 
>>> where trying to test libcxx is particularly difficult to do apparently.
>>
>> I disagree; testing libcxx is easy.
>
> FWIW, that's not been my experience. I can't even get libcxx to build for me 
> on Windows with Visual Studio cmake integration, let alone test it.

Fair point, I can't vouch for how well that build is supported, since I don't 
have access to Windows. This version isn't tested in libc++ CI. The other two 
Windows builds `CMake + ninja (MSVC)` and `CMake + ninja (MSVC)` are tested in 
libc++ CI. The wording on the website is similar to what is used in libc++ CI 
`libcxx/utils/ci/run-buildbot` but it misses the ` 
-DLIBCXX_EXTRA_SITE_DEFINES="_LIBCPP_HAS_NO_INT128"`.

>> Unfortunately what's missing is good documentation explaining how to test 
>> different configurations. A similar libc++ related issue came up in another 
>> Clang review recently where the libc++ CI setup was unclear. Afterwards I 
>> had a talk with @aaron.ballman and I agreed to improve the documentation. 
>> While improving the documentation I noticed some issues with our CI setup. 
>> So before publishing documentation that doesn't reflect reality, I first 
>> wanted to fix these issues. While testing my fixes I ran into the build 
>> failure due to this patch. So now we're at a full circle.
>
> And I greatly appreciate the improvements that have been going on here!

You're welcome!

In D126907#3739967 , @erichkeane 
wrote:

> In D126907#3739750 , @Mordante 
> wrote:
>
>> If you need further assistance feel free to reach out to me or other libc++ 
>> developers, the easiest way to reach us is #libcxx on Discord.
>
> This is the 3rd time some non-obvious-how-to-run libcxx test failure has 
> caused a revert request to this patch, which is obviously getting 
> frustrating. Additionally, in none of the 3 cases was I alerted automatically.

I understand that it's frustrating. I haven't seen the other two reverts, but 
by default non libc++ patches aren't tested in the libc++ pre-commit CI. The 
reason is simple; a pre-commit build runs 50+ builds and takes about one hour 
to complete. So the CI doesn't have the capacity to test every Clang diff. If 
you want to run the libc++ pre-commit CI the easiest way is to add a dummy file 
in the libc++ tree. Note that will add the libc++ review group to this review.

Next to testing pre-commit changes it tests main every 4 hours, but doesn't 
send e-mails when an error occurs. This is something @ldionne wanted to look 
into.

This behaviour is exactly the kind of documentation that is missing for Clang 
developers and what @aaron.ballman and I talked about. (There is more 
documentation that should be added, but that is generic information.)

> I appreciate that there are actually ways to RUN these tests on my machine 
> (Aaron is not as lucky, see above), but the lack of a "test all the libcxx 
> things" flag is shocking.  It seems that I'd need at least 2 separate CMake 
> directories to test these configurations?  That seems shocking to me.

You can use one libc++ installation to test multiple different configuration as 
long as they don't use different build-time options. So testing with the 
different language standards c++03, c++11, c++17, c++20, c++2b, the modular 
build, and several other options can be done from one build. But you need to 
invoke the tests for each option or combination of options separately.

If you want to test without `wchar_t` support you need to build a different 
libc++, simply since that removes parts of the dylib, like the `wchar_t` facets 
from the locales. (Basically the same as when you want to test Clang with or 
without assertions, there you need two builds too.)

What do you exactly mean with "test all the libcxx things" flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

FWIW, while I wasn't able to reproduce the problem that @Mordante reported, I 
found that the test suite as given has a large number of asserts in debug mode 
while trying to compare parameter-mappings during constraint 
normalization(assert is `clang-15: 
/iusers/ekeane1/workspaces/delayed-concepts/clang/lib/AST/ASTContext.cpp:4753: 
clang::QualType clang::ASTContext::getSubstTemplateTypeParmType(const 
clang::TemplateTypeParmType*, clang::QualType, llvm::Optional) 
const: Assertion ```Replacement.isCanonical() && "replacement types must always 
be canonical"' failed.```).`

One Reproducer is:

  #pragma clang module import std.array
  template
  struct iter_value_or_void { using type = void; };

  template
  struct iter_value_or_void {
  using type = std::iter_value_t;

BUT debugging seems to show a lot of 'imported' symbols that I'm not 
particularly sure how they work, so I suspect they are coming from the 
clang-module-import.

So I'm going to have to try to figure out what is going on here with the clang 
modules.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3739750 , @Mordante wrote:

> In D126907#3738383 , @erichkeane 
> wrote:
>
>> There was a test I dealt with previously where a ton of the header files 
>> were run with modules enabled (and an auto generated files), so I'm shocked 
>> to find there is MORE differences here.  @ldionne : This is another example 
>> where trying to test libcxx is particularly difficult to do apparently.
>
> I disagree; testing libcxx is easy. Unfortunately what's missing is good 
> documentation explaining how to test different configurations. A similar 
> libc++ related issue came up in another Clang review recently where the 
> libc++ CI setup was unclear. Afterwards I had a talk with @aaron.ballman and 
> I agreed to improve the documentation. While improving the documentation I 
> noticed some issues with our CI setup. So before publishing documentation 
> that doesn't reflect reality, I first wanted to fix these issues. While 
> testing my fixes I ran into the build failure due to this patch. So now we're 
> at a full circle.
>
> With the revert of this patch it's possible to fix the CI issues for libc++ 
> and I will continue with improving the documentation.
>
> Note that this specific issue isn't directly detected in the libc++ CI. This 
> would require a full clang build and testing with modules. That's not done, 
> full builds are only tested without modules. If wanted I supply a patch how 
> to test this configuration in the libc++ CI.
>
> If you need further assistance feel free to reach out to me or other libc++ 
> developers, the easiest way to reach us is #libcxx on Discord.

This is the 3rd time some non-obvious-how-to-run libcxx test failure has caused 
a revert request to this patch, which is obviously getting frustrating. 
Additionally, in none of the 3 cases was I alerted automatically.

I appreciate that there are actually ways to RUN these tests on my machine 
(Aaron is not as lucky, see above), but the lack of a "test all the libcxx 
things" flag is shocking.  It seems that I'd need at least 2 separate CMake 
directories to test these configurations?  That seems shocking to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126907#3739750 , @Mordante wrote:

> In D126907#3738383 , @erichkeane 
> wrote:
>
>> There was a test I dealt with previously where a ton of the header files 
>> were run with modules enabled (and an auto generated files), so I'm shocked 
>> to find there is MORE differences here.  @ldionne : This is another example 
>> where trying to test libcxx is particularly difficult to do apparently.
>
> I disagree; testing libcxx is easy.

FWIW, that's not been my experience. I can't even get libcxx to build for me on 
Windows with Visual Studio cmake integration, let alone test it. These are the 
results I got when I tried again today from the instructions from the website:

  CMake Error in F:/source/llvm-project/libcxx/benchmarks/CMakeLists.txt:
No known features for CXX compiler
  
"Clang"
  
version 16.0.0.
  
  
  CMake Generate step failed.  Build files cannot be regenerated correctly.
  FAILED: runtimes/runtimes-stamps/runtimes-configure 

That said, I recall at one point I was able to get libcxx to build for me, but 
then I ran into issues getting the tests to run (and we document this on the 
website: "Building with Visual Studio currently does not permit running 
tests."). The result is that I stopped trying to build or test libcxx locally 
ages ago and I rely entirely on the bots to tell me if something's gone wrong.

> Unfortunately what's missing is good documentation explaining how to test 
> different configurations. A similar libc++ related issue came up in another 
> Clang review recently where the libc++ CI setup was unclear. Afterwards I had 
> a talk with @aaron.ballman and I agreed to improve the documentation. While 
> improving the documentation I noticed some issues with our CI setup. So 
> before publishing documentation that doesn't reflect reality, I first wanted 
> to fix these issues. While testing my fixes I ran into the build failure due 
> to this patch. So now we're at a full circle.

And I greatly appreciate the improvements that have been going on here!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a subscriber: aaron.ballman.
Mordante added a comment.

In D126907#3738383 , @erichkeane 
wrote:

> There was a test I dealt with previously where a ton of the header files were 
> run with modules enabled (and an auto generated files), so I'm shocked to 
> find there is MORE differences here.  @ldionne : This is another example 
> where trying to test libcxx is particularly difficult to do apparently.

I disagree; testing libcxx is easy. Unfortunately what's missing is good 
documentation explaining how to test different configurations. A similar libc++ 
related issue came up in another Clang review recently where the libc++ CI 
setup was unclear. Afterwards I had a talk with @aaron.ballman and I agreed to 
improve the documentation. While improving the documentation I noticed some 
issues with our CI setup. So before publishing documentation that doesn't 
reflect reality, I first wanted to fix these issues. While testing my fixes I 
ran into the build failure due to this patch. So now we're at a full circle.

With the revert of this patch it's possible to fix the CI issues for libc++ and 
I will continue with improving the documentation.

Note that this specific issue isn't directly detected in the libc++ CI. This 
would require a full clang build and testing with modules. That's not done, 
full builds are only tested without modules. If wanted I supply a patch how to 
test this configuration in the libc++ CI.

If you need further assistance feel free to reach out to me or other libc++ 
developers, the easiest way to reach us is #libcxx on Discord.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a subscriber: ldionne.
erichkeane added a comment.

In D126907#3737986 , @Mordante wrote:

> In D126907#3737641 , @erichkeane 
> wrote:
>
>> In D126907#3737375 , @Mordante 
>> wrote:
>>
>>> This change breaks libc++'s modular build see build 
>>> https://buildkite.com/llvm-project/libcxx-ci/builds/12991
>>> Reverting this commit on top of yesterday's build (before your revert) 
>>> fixes the issue.
>>>
>>> libc++'s modular build uses Clang's pre-C++20 modules.
>>>
>>> It can be reproduced building libc++ and running the following lit 
>>> invocation
>>> `${LIT} -Denable_modules=True 
>>> libcxx/test/libcxx/ranges/range.adaptors/range.all/all.nodiscard.verify.cpp`
>>>
>>> If you need assistance testing a new version of this patch on libc++ please 
>>> let me know.
>>
>> Is this not part of check-runtimes or check-cxx?  I ran both of those 
>> INCLUDING a modules-builds-all...
>
> It should be tested in `check-cxx` when you use the cmake option 
> `-DLIBCXX_TEST_PARAMS="enable_modules=True"` and you need to configure libc++ 
> to use your just build clang to be used for the tests. (The command I posted 
> just does a single test, which is faster during bisecting.)
>
> I'm not sure what you mean with `modules-builds-all`.

There was a test I dealt with previously where a ton of the header files were 
run with modules enabled (and an auto generated files), so I'm shocked to find 
there is MORE differences here.  @ldionne : This is another example where 
trying to test libcxx is particularly difficult to do apparently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

In D126907#3737641 , @erichkeane 
wrote:

> In D126907#3737375 , @Mordante 
> wrote:
>
>> This change breaks libc++'s modular build see build 
>> https://buildkite.com/llvm-project/libcxx-ci/builds/12991
>> Reverting this commit on top of yesterday's build (before your revert) fixes 
>> the issue.
>>
>> libc++'s modular build uses Clang's pre-C++20 modules.
>>
>> It can be reproduced building libc++ and running the following lit invocation
>> `${LIT} -Denable_modules=True 
>> libcxx/test/libcxx/ranges/range.adaptors/range.all/all.nodiscard.verify.cpp`
>>
>> If you need assistance testing a new version of this patch on libc++ please 
>> let me know.
>
> Is this not part of check-runtimes or check-cxx?  I ran both of those 
> INCLUDING a modules-builds-all...

It should be tested in `check-cxx` when you use the cmake option 
`-DLIBCXX_TEST_PARAMS="enable_modules=True"` and you need to configure libc++ 
to use your just build clang to be used for the tests. (The command I posted 
just does a single test, which is faster during bisecting.)

I'm not sure what you mean with `modules-builds-all`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3737375 , @Mordante wrote:

> This change breaks libc++'s modular build see build 
> https://buildkite.com/llvm-project/libcxx-ci/builds/12991
> Reverting this commit on top of yesterday's build (before your revert) fixes 
> the issue.
>
> libc++'s modular build uses Clang's pre-C++20 modules.
>
> It can be reproduced building libc++ and running the following lit invocation
> `${LIT} -Denable_modules=True 
> libcxx/test/libcxx/ranges/range.adaptors/range.all/all.nodiscard.verify.cpp`
>
> If you need assistance testing a new version of this patch on libc++ please 
> let me know.

Is this not part of check-runtimes or check-cxx?  I ran both of those INCLUDING 
a modules-builds-all...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

This change breaks libc++'s modular build see build 
https://buildkite.com/llvm-project/libcxx-ci/builds/12991
Reverting this commit on top of yesterday's build (before your revert) fixes 
the issue.

libc++'s modular build uses Clang's pre-C++20 modules.

It can be reproduced building libc++ and running the following lit invocation
`${LIT} -Denable_modules=True 
libcxx/test/libcxx/ranges/range.adaptors/range.all/all.nodiscard.verify.cpp`

If you need assistance testing a new version of this patch on libc++ please let 
me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[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 rGd483730d8c3f: Re-apply Deferred Concept Instantiation 
Implementation (authored by erichkeane).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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/SemaExpr.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/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/SemaTemplate/concepts-friends.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 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

Respond/fix all of the things @ChuanqiXu mentioned.  Intend to commit early 
tomorrow based on latest feedback unless others have concerns.


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

https://reviews.llvm.org/D126907

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/SemaExpr.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/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/SemaTemplate/concepts-friends.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 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Sema/Sema.h:3642
+  // template, for the purposes of [temp.friend] p9.
+  bool ConstraintExpressionDependsOnEnclosingTemplate(unsigned TemplateDepth,
+  const Expr *Constraint);

ChuanqiXu wrote:
> The meaning of `TemplateDepth` is unclear. Do it mean top-down or start from 
> the constraint expression?
It is top-down, as the constraint is uninstantiated.  I've added a comment to 
clarify.



Comment at: clang/include/clang/Sema/Template.h:507-518
+struct ConstraintEvalRAII {
+  TemplateDeclInstantiator 
+  bool OldValue;
+
+  ConstraintEvalRAII(TemplateDeclInstantiator )
+  : TI(TI), OldValue(TI.EvaluateConstraints) {
+TI.EvaluateConstraints = false;

ChuanqiXu wrote:
> Could we remove the duplicates? For example, is it possible to make 
> ConstraintEvalRAII a subclass of Sema?
It ends up having to be a template, and for a 'getter' to exist, but done.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2786-2799
+template 
+static bool DeducedArgsNeedReplacement(TemplateDeclT *Template) {
+  return false;
+}
+template <>
+bool DeducedArgsNeedReplacement(
+VarTemplatePartialSpecializationDecl *Spec) {

ChuanqiXu wrote:
> nit:
These are specializations, so they cant have a storage class.  They inherit 
them from the primary template.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1233
 /* DeclContext *Owner */ Owner, TemplateArgs);
+  DeclInstantiator.setEvaluateConstraints(EvaluateConstraints);
+  return DeclInstantiator.SubstTemplateParams(OrigTPL);

ChuanqiXu wrote:
> Do we have any method to detect if the template parameter list lives in a 
> require clause or not? The current duplicating looks a little bit bad.
> 
> If there is no such methods, I guess it may be better by using enums:
> 
> ```
> TemplateParameterList *TransformTemplateParameterList(
>   TemplateParameterList *OrigTPL, enum 
> IsInRequire = Normal) {
>...
>if (IsInRequire == ...)
>   DeclInstantiator.setEvaluateConstraints(EvaluateConstraints);
>...
> }
> ```
Turns out I don't think this was required anyway, so I just removed it.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2435
+  CXXRecordDecl *Record = cast(DC);
+  Expr *TrailingRequiresClause = D->getTrailingRequiresClause();
+

ChuanqiXu wrote:
> Is this used?
Yes, see line 2453, 2459, 2470, and 2476.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I am not sure if I don't miss any points. But if it is OK to run 
libc++/libstdc++, I think it should be good to land this.




Comment at: clang/include/clang/Sema/Sema.h:3642
+  // template, for the purposes of [temp.friend] p9.
+  bool ConstraintExpressionDependsOnEnclosingTemplate(unsigned TemplateDepth,
+  const Expr *Constraint);

The meaning of `TemplateDepth` is unclear. Do it mean top-down or start from 
the constraint expression?



Comment at: clang/include/clang/Sema/Template.h:507-518
+struct ConstraintEvalRAII {
+  TemplateDeclInstantiator 
+  bool OldValue;
+
+  ConstraintEvalRAII(TemplateDeclInstantiator )
+  : TI(TI), OldValue(TI.EvaluateConstraints) {
+TI.EvaluateConstraints = false;

Could we remove the duplicates? For example, is it possible to make 
ConstraintEvalRAII a subclass of Sema?



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2786-2799
+template 
+static bool DeducedArgsNeedReplacement(TemplateDeclT *Template) {
+  return false;
+}
+template <>
+bool DeducedArgsNeedReplacement(
+VarTemplatePartialSpecializationDecl *Spec) {

nit:



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1233
 /* DeclContext *Owner */ Owner, TemplateArgs);
+  DeclInstantiator.setEvaluateConstraints(EvaluateConstraints);
+  return DeclInstantiator.SubstTemplateParams(OrigTPL);

Do we have any method to detect if the template parameter list lives in a 
require clause or not? The current duplicating looks a little bit bad.

If there is no such methods, I guess it may be better by using enums:

```
TemplateParameterList *TransformTemplateParameterList(
  TemplateParameterList *OrigTPL, enum IsInRequire 
= Normal) {
   ...
   if (IsInRequire == ...)
  DeclInstantiator.setEvaluateConstraints(EvaluateConstraints);
   ...
}
```



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3609-3610
+
+  // TODO: ERICH: This is where we need to make sure we 'know' constraint
+  // checking needs to happen.
+  TemplateInstantiator Instantiator(*this, TemplateArgs, SourceLocation(),





Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2435
+  CXXRecordDecl *Record = cast(DC);
+  Expr *TrailingRequiresClause = D->getTrailingRequiresClause();
+

Is this used?


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

SO, I charted the differences in runtimes, and my patch is BETTER time-wise in 
release mode, but WORSE with asserts enabled.  I don't know of any expensive 
assert that I added.  SO, if everyone could review this, I'd love to get this 
committed to see if it beats the buildbots this time:)


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think I've proved to myself that there IS a compile-time regression, but it 
is reasonably minor.  I suspect a lot of it is going to be the 'friend' 
comparisons not caching, which I might look into doing.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3715436 , @ChuanqiXu wrote:

> In D126907#3712363 , @erichkeane 
> wrote:
>
>> In D126907#3711956 , @ChuanqiXu 
>> wrote:
>>
>>> In D126907#3710656 , @erichkeane 
>>> wrote:
>>>
 Ok, fixed the test failure in clang, AND it managed to fix 
 all the failures in libcxx!

 HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
 is now hanging?

 I don't know much about the modules implementation (perhaps someone
 like @ChuanqiXu  can help out?), so I'm at least somewhat stuck until
 I can figure out how to get it to give me more info.
>>>
>>> I may not be able to look into the details recently. What's the error 
>>> message from the modules?
>>
>> Looking deeper... this test is a monstrosity 
>> (https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/modules_include.sh.cpp),
>>  and I'm not necessarily sure it is necessarily modules-related, other than 
>> enabling it on every header.  So it just seems like one of the STL headers 
>> is taking significantly longer to compile?  I have no idea how to move 
>> forward with this... it AT LEAST "finishes", but it takes a very long time 
>> to get through now.
>
> I tried to reproduce and I am not sure if reproduced. With this patch, 
> `modules_include.sh.cpp` takes 562s to complete. And without this patch, it 
> takes a 557s. So it looks not your fault.

Hmm... ok, thanks for checking!  I'm going to try to prove to myself that it 
isn't too bad then.  This might just be a case of the Debug version of this 
taking a horribly long-time and me not noticing it normally.

If that is the case, I think this is ready for review!


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3712363 , @erichkeane 
wrote:

> In D126907#3711956 , @ChuanqiXu 
> wrote:
>
>> In D126907#3710656 , @erichkeane 
>> wrote:
>>
>>> Ok, fixed the test failure in clang, AND it managed to fix 
>>> all the failures in libcxx!
>>>
>>> HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
>>> is now hanging?
>>>
>>> I don't know much about the modules implementation (perhaps someone
>>> like @ChuanqiXu  can help out?), so I'm at least somewhat stuck until
>>> I can figure out how to get it to give me more info.
>>
>> I may not be able to look into the details recently. What's the error 
>> message from the modules?
>
> Looking deeper... this test is a monstrosity 
> (https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/modules_include.sh.cpp),
>  and I'm not necessarily sure it is necessarily modules-related, other than 
> enabling it on every header.  So it just seems like one of the STL headers is 
> taking significantly longer to compile?  I have no idea how to move forward 
> with this... it AT LEAST "finishes", but it takes a very long time to get 
> through now.

I tried to reproduce and I am not sure if reproduced. With this patch, 
`modules_include.sh.cpp` takes 562s to complete. And without this patch, it 
takes a 557s. So it looks not your fault.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3711956 , @ChuanqiXu wrote:

> In D126907#3710656 , @erichkeane 
> wrote:
>
>> Ok, fixed the test failure in clang, AND it managed to fix 
>> all the failures in libcxx!
>>
>> HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
>> is now hanging?
>>
>> I don't know much about the modules implementation (perhaps someone
>> like @ChuanqiXu  can help out?), so I'm at least somewhat stuck until
>> I can figure out how to get it to give me more info.
>
> I may not be able to look into the details recently. What's the error message 
> from the modules?

Looking deeper... this test is a monstrosity 
(https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/modules_include.sh.cpp),
 and I'm not necessarily sure it is necessarily modules-related, other than 
enabling it on every header.  So it just seems like one of the STL headers is 
taking significantly longer to compile?  I have no idea how to move forward 
with this... it AT LEAST "finishes", but it takes a very long time to get 
through now.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3711956 , @ChuanqiXu wrote:

> In D126907#3710656 , @erichkeane 
> wrote:
>
>> Ok, fixed the test failure in clang, AND it managed to fix 
>> all the failures in libcxx!
>>
>> HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
>> is now hanging?
>>
>> I don't know much about the modules implementation (perhaps someone
>> like @ChuanqiXu  can help out?), so I'm at least somewhat stuck until
>> I can figure out how to get it to give me more info.
>
> I may not be able to look into the details recently. What's the error message 
> from the modules?

At the moment, there IS no error message!  Just that the modules_include.sh.cpp 
test now takes a REALLY long time (I thought it was a 'hang', but it finished 
overnight).  So it is more like an extreme compile-time regression.  I can't 
make heads or tails of what the test is SUPPOSED to be doing, so I don't have a 
good idea what the issue is, nor what is happening...


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3710656 , @erichkeane 
wrote:

> Ok, fixed the test failure in clang, AND it managed to fix 
> all the failures in libcxx!
>
> HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
> is now hanging?
>
> I don't know much about the modules implementation (perhaps someone
> like @ChuanqiXu  can help out?), so I'm at least somewhat stuck until
> I can figure out how to get it to give me more info.

I may not be able to look into the details recently. What's the error message 
from the modules?


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

Ok, fixed the test failure in clang, AND it managed to fix 
all the failures in libcxx!

HOWEVER, it appears that libcxx/test/libcxx/modules_include.sh.cpp
is now hanging?

I don't know much about the modules implementation (perhaps someone
like @ChuanqiXu  can help out?), so I'm at least somewhat stuck until
I can figure out how to get it to give me more info.


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

https://reviews.llvm.org/D126907

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/SemaExpr.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/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/SemaTemplate/concepts-friends.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 @@
 
 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

I pulled out a good amount of this patch as NFC, which should shrink the patch.

I have a 'new approach' that I think will work, which is to suppress the 
constraint
evaluation the same way we did with the requires clause (though, with a flag, 
rather than just being able to skip it).

At the moment, I have 1 lit test failure in Clang.  Additionally, I have 2-3 
libcxx
failures where we seem to static-assert where we didn't before, PLUS 1 test 
that seems
to hang.  I'm hopeful I am on the right track and can continue to make progress.


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

https://reviews.llvm.org/D126907

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/SemaExpr.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/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/SemaTemplate/concepts-friends.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 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3667265 , @ChuanqiXu wrote:

> In D126907#3665900 , @erichkeane 
> wrote:
>
>> The more I look at this... the more I question my approach.  I now am 
>> definitely sure we won't make Clang15, and hope that I can figure something 
>> better out for 16 :/
>
> I feel like it could be helpful to split it into several small patches next 
> time.

I REALLY wish I knew how to make that happen.  The problem is the main idea 
(deferring instantiation) is an 'all or nothing' that really seems to break 
things.  There are a handful of small-ish 'cleanup' tasks that I did along the 
way, and could probably come up with another, but I'm not sure how much I can 
remove from the patch to split it up :/


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3665900 , @erichkeane 
wrote:

> The more I look at this... the more I question my approach.  I now am 
> definitely sure we won't make Clang15, and hope that I can figure something 
> better out for 16 :/

I feel like it could be helpful to split it into several small patches next 
time.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

The more I look at this... the more I question my approach.  I now am 
definitely sure we won't make Clang15, and hope that I can figure something 
better out for 16 :/


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Hmm... I think my approach was slightly off... I have to spend more time on 
this, but thanks for the review!  I hope I'm learning each time we go again :/


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Ah... I remember now, and I think that solution isn't quite right.  The idea is 
that we want to defer constraint evaluation when the constraint is on a 
function decl/template decl.  The PROBLEM I need to solve is when it is in a 
body of a struct or function.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:963-964
+  Entity(Entity),
+  EvaluatingAConstraint(EvaluatingConstraint ||
+!SemaRef.CurContext->isDependentContext()) {}
 

ChuanqiXu wrote:
> erichkeane wrote:
> > ChuanqiXu wrote:
> > > I would like to see this in call site and a comment to tell us that we 
> > > don't want to evaluate constraints in dependent context.
> > So you want this logic moved to the call-sites?  Or am I misinterpreting 
> > that?
> Yes. It is odd **for me** to do logic operations in the initialization list.
Got it, that makes sense.

Though when looking at it, I found myself wondering why I did this change, it 
seems to 'undo' a lot of the deferred concepts work :/


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:963-964
+  Entity(Entity),
+  EvaluatingAConstraint(EvaluatingConstraint ||
+!SemaRef.CurContext->isDependentContext()) {}
 

erichkeane wrote:
> ChuanqiXu wrote:
> > I would like to see this in call site and a comment to tell us that we 
> > don't want to evaluate constraints in dependent context.
> So you want this logic moved to the call-sites?  Or am I misinterpreting that?
Yes. It is odd **for me** to do logic operations in the initialization list.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:963-964
+  Entity(Entity),
+  EvaluatingAConstraint(EvaluatingConstraint ||
+!SemaRef.CurContext->isDependentContext()) {}
 

ChuanqiXu wrote:
> I would like to see this in call site and a comment to tell us that we don't 
> want to evaluate constraints in dependent context.
So you want this logic moved to the call-sites?  Or am I misinterpreting that?


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3645198 , @erichkeane 
wrote:

> In D126907#3644127 , @ChuanqiXu 
> wrote:
>
>> In D126907#3642530 , @erichkeane 
>> wrote:
>>
>>> This version passes check-runtimes, so libc++ is fine, and it passes 
>>> everything I have available.  @ChuanqiXu : Would you be able to do 1 more 
>>> run over this to make sure it won't break something?  Thanks in advance 
>>> either way!
>>
>> I've tested some our internal workloads and all of them looks fine. But it 
>> is expected since we don't use concept heavily. I had tried to run libcxx 
>> before but it looks like my previous config is wrong...
>
> Ok, thanks!  I DID test the libcxx test suite on the above, I figured it out 
> through check-runtimes.  If you were able to take a quick look at the diff 
> and make sure I'm not doing anything horrible, it would be appreciated.

Yeah, things looks good to me basically.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:963-964
+  Entity(Entity),
+  EvaluatingAConstraint(EvaluatingConstraint ||
+!SemaRef.CurContext->isDependentContext()) {}
 

I would like to see this in call site and a comment to tell us that we don't 
want to evaluate constraints in dependent context.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3644127 , @ChuanqiXu wrote:

> In D126907#3642530 , @erichkeane 
> wrote:
>
>> This version passes check-runtimes, so libc++ is fine, and it passes 
>> everything I have available.  @ChuanqiXu : Would you be able to do 1 more 
>> run over this to make sure it won't break something?  Thanks in advance 
>> either way!
>
> I've tested some our internal workloads and all of them looks fine. But it is 
> expected since we don't use concept heavily. I had tried to run libcxx before 
> but it looks like my previous config is wrong...

Ok, thanks!  I DID test the libcxx test suite on the above, I figured it out 
through check-runtimes.  If you were able to take a quick look at the diff and 
make sure I'm not doing anything horrible, it would be appreciated.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3642530 , @erichkeane 
wrote:

> This version passes check-runtimes, so libc++ is fine, and it passes 
> everything I have available.  @ChuanqiXu : Would you be able to do 1 more run 
> over this to make sure it won't break something?  Thanks in advance either 
> way!

I've tested some our internal workloads and all of them looks fine. But it is 
expected since we don't use concept heavily. I had tried to run libcxx before 
but it looks like my previous config is wrong...


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3636830 , @erichkeane 
wrote:

> SO I've been playing with this for a while and all the libcxx issues. I THINK 
> we really need to just bit the bullet and figure out how to correctly re-add 
> things to the instantiation scope (after making the CheckFunctionConstraints 
> LocalInstantiationScope 'false' for the 2nd param).  I can do it well enough 
> with functions since we have a function for it, but likely need to do the 
> same for at least RecordDecl.

URGH And its not clear to me that I CAN do this, since a containing 
function template's body doesn't exist yet, so we cant go back and make those 
work.  So we are at a point where we both have to inherit AND not inherit the 
paretn scope, and I have no idea on which basis to do so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

SO I've been playing with this for a while and all the libcxx issues. I THINK 
we really need to just bit the bullet and figure out how to correctly re-add 
things to the instantiation scope (after making the CheckFunctionConstraints 
LocalInstantiationScope 'false' for the 2nd param).  I can do it well enough 
with functions since we have a function for it, but likely need to do the same 
for at least RecordDecl.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Welp, fixed THIS problem but lead to another issue in libc++ that I'm still 
running down.  Stand by all :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Vitaly Buka via Phabricator via cfe-commits]

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

In D126907#3623377 , @vitalybuka 
wrote:

> FYI. The same on a different bot 
> https://lab.llvm.org/buildbot/#/builders/5/builds/25486/steps/19/logs/stdio

Not sure if the URL above was not related, as lease I see no red builds after 
"relanded patch"

However these are 100% relevant, I tested msan one locally 258c3aee54e1 
 vs 
188582b7e0f3 

https://lab.llvm.org/buildbot/#/builders/74/builds/11706
https://lab.llvm.org/buildbot/#/builders/85/builds/8938
https://lab.llvm.org/buildbot/#/builders/168/builds/7114

However sanitizers has nothing to do with the problems, you just need compilers 
builds with asserts, and then use that one to build and run libcxx tests.

This reproduces the issue on 188582b7e0f357bb9b137b995f773d114472a9c4 


  mkdir b0
  cd b0/
  # ${HOME}/src/clang/bin/clang++ is some stable good build of clang.
  cmake -GNinja ${HOME}/src/llvm.git/llvm-project/llvm 
'-DLLVM_ENABLE_PROJECTS=clang;lld'   -DLLVM_CCACHE_BUILD=ON 
-DCMAKE_C_COMPILER=${HOME}/src/clang/bin/clang 
-DCMAKE_CXX_COMPILER=${HOME}/src/clang/bin/clang++  -DCMAKE_BUILD_TYPE=Release 
-DLLVM_ENABLE_ASSERTIONS=ON
  ninja
  
  mkdir ../b1
  cd ../b1
  cmake -GNinja ${HOME}/src/llvm.git/llvm-project/llvm 
'-DLLVM_ENABLE_PROJECTS=libcxx;libcxxabi'  -DCMAKE_C_COMPILER=$(readlink -f 
../b0)/bin/clang -DCMAKE_CXX_COMPILER=$(readlink -f ../b0)/bin/clang++  
-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON
  ninja check-cxx


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Crash that required the revert : 
https://reviews.llvm.org/rGbefa8cf087dbb8159a4d9dc8fa4d6748d6d5049a

reduces to :

   template
   concept sentinel_for =
 requires(_Ip __i) {
   __i++;
 };
  
   template
   concept bidirectional_iterator =
 sentinel_for<_Ip>;
  
   
  template 
  class move_iterator
  {
  public:
  auto operator++(int)
  requires
  sentinel_for<_Iter>;
  };
   
  static_assert( bidirectional_iterator>);

In other news, I discovered that 'check-all' and 'stage2-check-all' doesn't 
include 'check-runtimes', which managed to be why I missed this :/

I won't have time to poke that this until Tuesday since Monday is a US holiday 
and I'm done for the day, so if @ChuanqiXu or others wish to mess with this, 
feel free, otherwise I'll get back on it next week!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3625600 , @vitalybuka 
wrote:

> @erichkeane can you please make sure that committed revisions in git have 
> "Differential Revision: ". Usually it's added by "arc" when you upload 
> review.
> Also it would be nice if reapply have the same link.
>
> Regarding reproducer, you can follow 
> https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild
> But it's already related, let see if it resolved for our bot as well.

Yep, I'll make sure to remember that next time.  Corporate IT prevents me from 
using arc, so I am stuck doing it m anually and forget sometimes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

@erichkeane can you please make sure that committed revisions in git have 
"Differential Revision: ". Usually it's added by "arc" when you upload 
review.
Also it would be nice if reapply have the same link.

Regarding reproducer, you can follow 
https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild
But it's already related, let see if it resolved for our bot as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

@shafik  was able to help me figure out how to repro, and I think I've about 
fixed this, so I'll try another commit soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

I tried the ninja stage2-check-all on my local build with this patch applied 
and don't reproduce the issue.  Is there any way someone could get me the 
preprocessed source that this crashed on off the servers or something?  
@JDevlieghere or @vitalybuka  ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

> Yikes!  Thanks for the revert.  I didn't see the email from the bot until 
> just about 2 minutes before you reverted.  I'll see if I can reproduce.

FYI. The same on a different bot 
https://lab.llvm.org/buildbot/#/builders/5/builds/25486/steps/19/logs/stdio


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3623120 , @JDevlieghere 
wrote:

> This change triggers an assertion when building an LLDB test case:
>
>   UNREACHABLE executed at 
> /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/SemaTemplate.cpp:1726!
>   Assertion failed: (InstantiatingSpecializations.empty() && "failed to clean 
> up an InstantiatingTemplate?"), function ~Sema, file 
> /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/Sema.cpp,
>  line 458.
>   PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
> and include the crash backtrace, preprocessed source, and associated run 
> script.
>   Stack dump:
>   0.  Program arguments: 
> /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang -fmodules 
> -gmodules 
> -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api
>  -gmodules -fcxx-modules -std=c++11 -g -O0 -isysroot 
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk
>  -arch x86_64 
> -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/../../../../../include
>  
> -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span
>  
> -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make
>  -include 
> /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/test_common.h
>  -fno-limit-debug-info -fmodules -gmodules 
> -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api
>  -gmodules -fcxx-modules -DLLDB_USING_LIBCPP -stdlib=libc++ -std=c++20 
> --driver-mode=g++ -MT main.o -MD -MP -MF main.d -c -o main.o 
> /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp
>   1.  
> /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp:1:2:
>  current parser token 'include'
>   Stack dump without symbol names (ensure you have llvm-symbolizer in your 
> PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
>   0  clang0x000106bc157e 
> llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 46
>   1  clang0x000106bc0258 
> llvm::sys::RunSignalHandlers() + 248
>   2  clang0x000106bc0962 
> llvm::sys::CleanupOnSignal(unsigned long) + 210
>   3  clang0x000106adb82a (anonymous 
> namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) + 106
>   4  clang0x000106adba2e 
> CrashRecoverySignalHandler(int) + 110
>   5  libsystem_platform.dylib 0x7fff67b405fd _sigtramp + 29
>   6  libsystem_platform.dylib 0x7ffeeab5b460 _sigtramp + 
> 18446744071612509824
>   7  libsystem_c.dylib0x7fff67a12808 abort + 120
>   8  libsystem_c.dylib0x7fff67a11ac6 err + 0
>   9  clang0x000108a1325c clang::Sema::~Sema() + 5660
>   10 clang0x000107698e41 
> clang::CompilerInstance::~CompilerInstance() + 625
>   11 clang0x0001076a7fe5 
> compileModuleImpl(clang::CompilerInstance&, clang::SourceLocation, 
> llvm::StringRef, clang::FrontendInputFile, llvm::StringRef, llvm::StringRef, 
> llvm::function_ref, llvm::function_ref (clang::CompilerInstance&)>) + 4517
>   12 clang0x0001076a9b16 
> compileModuleAndReadASTImpl(clang::CompilerInstance&, clang::SourceLocation, 
> clang::SourceLocation, clang::Module*, llvm::StringRef) + 1238
>   13 clang0x0001076a370b 
> compileModuleAndReadAST(clang::CompilerInstance&, clang::SourceLocation, 
> clang::SourceLocation, clang::Module*, llvm::StringRef) + 1947
>   14 clang0x0001076a2b97 
> clang::CompilerInstance::findOrCompileModuleAndReadAST(llvm::StringRef, 
> clang::SourceLocation, clang::SourceLocation, bool) + 3783
>   15 clang0x0001076a3aa7 
> clang::CompilerInstance::loadModule(clang::SourceLocation, 
> llvm::ArrayRef clang::SourceLocation>>, clang::Module::NameVisibilityKind, bool) + 695
>   16 clang0x000109b8a630 
> clang::Preprocessor::HandleHeaderIncludeOrImport(clang::SourceLocation, 
> clang::Token&, clang::Token&, clang::SourceLocation, 
> clang::detail::SearchDirIteratorImpl, clang::FileEntry const*) + 7792
>   17 clang0x000109b822e1 
> clang::Preprocessor::HandleIncludeDirective(clang::SourceLocation, 
> clang::Token&, clang::detail::SearchDirIteratorImpl, clang::FileEntry 
> 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added a comment.

This change triggers an assertion when building an LLDB test case:

  UNREACHABLE executed at 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/SemaTemplate.cpp:1726!
  Assertion failed: (InstantiatingSpecializations.empty() && "failed to clean 
up an InstantiatingTemplate?"), function ~Sema, file 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/clang/lib/Sema/Sema.cpp,
 line 458.
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.Program arguments: 
/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang -fmodules 
-gmodules 
-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api
 -gmodules -fcxx-modules -std=c++11 -g -O0 -isysroot 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk
 -arch x86_64 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/../../../../../include
 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span
 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make
 -include 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/make/test_common.h
 -fno-limit-debug-info -fmodules -gmodules 
-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api
 -gmodules -fcxx-modules -DLLDB_USING_LIBCPP -stdlib=libc++ -std=c++20 
--driver-mode=g++ -MT main.o -MD -MP -MF main.d -c -o main.o 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp
  1.
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/span/main.cpp:1:2:
 current parser token 'include'
  Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH 
or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
  0  clang0x000106bc157e 
llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 46
  1  clang0x000106bc0258 llvm::sys::RunSignalHandlers() 
+ 248
  2  clang0x000106bc0962 
llvm::sys::CleanupOnSignal(unsigned long) + 210
  3  clang0x000106adb82a (anonymous 
namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) + 106
  4  clang0x000106adba2e 
CrashRecoverySignalHandler(int) + 110
  5  libsystem_platform.dylib 0x7fff67b405fd _sigtramp + 29
  6  libsystem_platform.dylib 0x7ffeeab5b460 _sigtramp + 
18446744071612509824
  7  libsystem_c.dylib0x7fff67a12808 abort + 120
  8  libsystem_c.dylib0x7fff67a11ac6 err + 0
  9  clang0x000108a1325c clang::Sema::~Sema() + 5660
  10 clang0x000107698e41 
clang::CompilerInstance::~CompilerInstance() + 625
  11 clang0x0001076a7fe5 
compileModuleImpl(clang::CompilerInstance&, clang::SourceLocation, 
llvm::StringRef, clang::FrontendInputFile, llvm::StringRef, llvm::StringRef, 
llvm::function_ref, llvm::function_ref) + 4517
  12 clang0x0001076a9b16 
compileModuleAndReadASTImpl(clang::CompilerInstance&, clang::SourceLocation, 
clang::SourceLocation, clang::Module*, llvm::StringRef) + 1238
  13 clang0x0001076a370b 
compileModuleAndReadAST(clang::CompilerInstance&, clang::SourceLocation, 
clang::SourceLocation, clang::Module*, llvm::StringRef) + 1947
  14 clang0x0001076a2b97 
clang::CompilerInstance::findOrCompileModuleAndReadAST(llvm::StringRef, 
clang::SourceLocation, clang::SourceLocation, bool) + 3783
  15 clang0x0001076a3aa7 
clang::CompilerInstance::loadModule(clang::SourceLocation, 
llvm::ArrayRef>, 
clang::Module::NameVisibilityKind, bool) + 695
  16 clang0x000109b8a630 
clang::Preprocessor::HandleHeaderIncludeOrImport(clang::SourceLocation, 
clang::Token&, clang::Token&, clang::SourceLocation, 
clang::detail::SearchDirIteratorImpl, clang::FileEntry const*) + 7792
  17 clang0x000109b822e1 
clang::Preprocessor::HandleIncludeDirective(clang::SourceLocation, 
clang::Token&, clang::detail::SearchDirIteratorImpl, clang::FileEntry 
const*) + 177
  18 clang0x000109b82f8c 
clang::Preprocessor::HandleDirective(clang::Token&) + 2604
  19 clang0x000109b50877 
clang::Lexer::LexTokenInternal(clang::Token&, bool) + 6199
  20 clang0x000109b4cf15 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[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 rG2f207439521d: 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/D126907/new/

https://reviews.llvm.org/D126907

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/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/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/SemaTemplate/concepts-friends.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 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/AST/Decl.h:1944-1948
+  /// For non-templates this value will be NULL, unless this non-template
+  /// function declaration was declared directly inside of a function template,
+  /// in which case this will have a pointer to a FunctionDecl, stored in the
+  /// NamedDecl. For function declarations that describe a function template,
+  /// this will be a pointer to a FunctionTemplateDecl, stored in the 
NamedDecl.

ChuanqiXu wrote:
> erichkeane wrote:
> > ChuanqiXu wrote:
> > > Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? 
> > > The NamedDecl* covers a lot of things. It looks more consistent.
> > All of those 'Info' types contain significantly more information, which we 
> > really don't have a need for here.  Basically all this patch is doing is 
> > using the FunctionTemplateDecl* that was already in the list and 
> > generalizing it a bit.  AND unfortunately, the only commonality between 
> > FunctionTemplateDecl and FunctionDecl is NamedDecl.  
> > 
> > So any type we would use would end up causing an additional allocation 
> > here, and make its use require us to unpack it :/
> > 
> > I guess what I'm saying, is I'm not sure what that would look like in a way 
> > that wouldn't make this worse.  Do you have an idea you could share that 
> > would improve that?  
> > 
> > 
> My idea would be something like:
> 
> ```C++
> llvm::PointerUnion MemberSpecializationInfo *,
>  FunctionTemplateSpecializationInfo *,
> ```
> 
> > So any type we would use would end up causing an additional allocation 
> > here, and make its use require us to unpack it :/
> 
> This is a union so it wouldn't cause additional allocations?
> 
I tried doing FunctionDecl and FunctionTemplateDecl at one point, but we endd 
up running out of bits for the pointer-union to work on 32 bit builds.  
Otherwise I would have done that.

I misunderstood the 'Info' thing and thought you wanted a new wrapper type (an 
'Info' type), which would need to be allocated and contain just a pointer, 
which is why I was thinking about needing allocations.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

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

In D126907#3619270 , @erichkeane 
wrote:

> All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the 
> 'info'.

LGTM. But the 'info' comment doesn't matter a lot. The current complexity is 
acceptable and its semantics is stated clearly in the comments. My intention is 
to make its semantics more clear and more readable so that it is easier to 
maintain and change it. But this is required. Since the patch is really large. 
So my preference is to do other unnecessary changes in other revision to make 
the patch easier to read. (Just a preference. I know some people prefer large 
patches.)




Comment at: clang/include/clang/AST/Decl.h:1944-1948
+  /// For non-templates this value will be NULL, unless this non-template
+  /// function declaration was declared directly inside of a function template,
+  /// in which case this will have a pointer to a FunctionDecl, stored in the
+  /// NamedDecl. For function declarations that describe a function template,
+  /// this will be a pointer to a FunctionTemplateDecl, stored in the 
NamedDecl.

erichkeane wrote:
> ChuanqiXu wrote:
> > Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? 
> > The NamedDecl* covers a lot of things. It looks more consistent.
> All of those 'Info' types contain significantly more information, which we 
> really don't have a need for here.  Basically all this patch is doing is 
> using the FunctionTemplateDecl* that was already in the list and generalizing 
> it a bit.  AND unfortunately, the only commonality between 
> FunctionTemplateDecl and FunctionDecl is NamedDecl.  
> 
> So any type we would use would end up causing an additional allocation here, 
> and make its use require us to unpack it :/
> 
> I guess what I'm saying, is I'm not sure what that would look like in a way 
> that wouldn't make this worse.  Do you have an idea you could share that 
> would improve that?  
> 
> 
My idea would be something like:

```C++
llvm::PointerUnion So any type we would use would end up causing an additional allocation here, 
> and make its use require us to unpack it :/

This is a union so it wouldn't cause additional allocations?



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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the 
'info'.


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

https://reviews.llvm.org/D126907

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/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/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/SemaTemplate/concepts-friends.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 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 9 inline comments as done.
erichkeane added a comment.

In D126907#3617713 , @ChuanqiXu wrote:

> In D126907#3615807 , @erichkeane 
> wrote:
>
>> All tests pass now, I was able to get the template-template checks working 
>> correctly, and it passes all the tests I have available.  @ChuanqiXu if you 
>> could review/run what tests you have, it would be greatly appreciated.
>
> I've tested some of our workloads and libunifex (I know it contains a lot of 
> uses for concept). And all the tests passed now. So it looks like it wouldn't 
> cause regression failure.
>
> The implementation **basically** looks good to me. (This is really large and 
> I can't be sure I don't miss something). Only some minor issues remained.

Thank you SO much for your testing and reviews, I very much appreciate all the 
help you've given me.  I'm sorry that this has become such a huge patch, I 
really wish it was something I could have done in smaller increments, but that 
doesn't seem like it is possible since it is changing something so fundamental, 
and the fallout from that is huge.




Comment at: clang/include/clang/AST/Decl.h:1944-1948
+  /// For non-templates this value will be NULL, unless this non-template
+  /// function declaration was declared directly inside of a function template,
+  /// in which case this will have a pointer to a FunctionDecl, stored in the
+  /// NamedDecl. For function declarations that describe a function template,
+  /// this will be a pointer to a FunctionTemplateDecl, stored in the 
NamedDecl.

ChuanqiXu wrote:
> Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? The 
> NamedDecl* covers a lot of things. It looks more consistent.
All of those 'Info' types contain significantly more information, which we 
really don't have a need for here.  Basically all this patch is doing is using 
the FunctionTemplateDecl* that was already in the list and generalizing it a 
bit.  AND unfortunately, the only commonality between FunctionTemplateDecl and 
FunctionDecl is NamedDecl.  

So any type we would use would end up causing an additional allocation here, 
and make its use require us to unpack it :/

I guess what I'm saying, is I'm not sure what that would look like in a way 
that wouldn't make this worse.  Do you have an idea you could share that would 
improve that?  





Comment at: clang/include/clang/Sema/Sema.h:7056-7067
+  /*
+  // Keep track of whether we are evaluating a constraint.
+  unsigned ConstraintEvaluationDepth = 0;
+
+  class ConstraintEvalRAII {
+Sema 
+

ChuanqiXu wrote:
> Remove this before landing.
Yikes!  Thanks for catching this!



Comment at: clang/lib/AST/Decl.cpp:3788
  "Member function is already a specialization");
-  TemplateOrSpecialization = Template;
+  TemplateOrSpecialization = static_cast(Template);
+}

ChuanqiXu wrote:
> `static_cast` is rarely used in clang/LLVM to me. I know the reason is that 
> `TemplateOrSpecialization` contains a `NamedDecl*` member. I guess it might 
> be better to refactor it.
Ah, looks like the static-casts weren't necessary anyway.  No idea why I 
thought they were.  Removed.  See comment above about the refactor.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3615807 , @erichkeane 
wrote:

> All tests pass now, I was able to get the template-template checks working 
> correctly, and it passes all the tests I have available.  @ChuanqiXu if you 
> could review/run what tests you have, it would be greatly appreciated.

I've tested some of our workloads and libunifex (I know it contains a lot of 
uses for concept). And all the tests passed now. So it looks like it wouldn't 
cause regression failure.

The implementation **basically** looks good to me. (This is really large and I 
can't be sure I don't miss something). Only some minor issues remained.




Comment at: clang/include/clang/AST/Decl.h:1944-1948
+  /// For non-templates this value will be NULL, unless this non-template
+  /// function declaration was declared directly inside of a function template,
+  /// in which case this will have a pointer to a FunctionDecl, stored in the
+  /// NamedDecl. For function declarations that describe a function template,
+  /// this will be a pointer to a FunctionTemplateDecl, stored in the 
NamedDecl.

Could we give a new abstract like `XXXInfo` to replace `NamedDecl*` here? The 
NamedDecl* covers a lot of things. It looks more consistent.



Comment at: clang/include/clang/Sema/Sema.h:7056-7067
+  /*
+  // Keep track of whether we are evaluating a constraint.
+  unsigned ConstraintEvaluationDepth = 0;
+
+  class ConstraintEvalRAII {
+Sema 
+

Remove this before landing.



Comment at: clang/include/clang/Sema/Sema.h:7070
 public:
+  // bool IsEvaluatingAConstraint() { return ConstraintEvaluationDepth > 0; }
   const NormalizedConstraint *

ditto



Comment at: clang/lib/AST/Decl.cpp:3788
  "Member function is already a specialization");
-  TemplateOrSpecialization = Template;
+  TemplateOrSpecialization = static_cast(Template);
+}

`static_cast` is rarely used in clang/LLVM to me. I know the reason is that 
`TemplateOrSpecialization` contains a `NamedDecl*` member. I guess it might be 
better to refactor it.



Comment at: clang/lib/AST/DeclBase.cpp:286-304
 const DeclContext *Decl::getParentFunctionOrMethod() const {
   for (const DeclContext *DC = getDeclContext();
DC && !DC->isTranslationUnit() && !DC->isNamespace();
DC = DC->getParent())
 if (DC->isFunctionOrMethod())
   return DC;
 

How about:



Comment at: clang/lib/Sema/SemaConcept.cpp:63
+
+  ExprResult recreateBinOp(Sema , ExprResult LHS) {
+return recreateBinOp(SemaRef, LHS, const_cast(getRHS()));





Comment at: clang/lib/Sema/SemaConcept.cpp:67
+
+  ExprResult recreateBinOp(Sema , ExprResult LHS, ExprResult RHS) {
+assert((isAnd() || isOr()) && "Not the right kind of op?");





Comment at: clang/lib/Sema/SemaConcept.cpp:186
+return BO.recreateBinOp(S, LHSRes, RHSRes);
   } else if (auto *C = dyn_cast(ConstraintExpr)) {
+// These aren't evaluated, so we don't care about cleanups, so we can just





Comment at: clang/lib/Sema/SemaTemplate.cpp:2339
   if (const auto *TC = TTP->getTypeConstraint())
-SemaRef.SubstTypeConstraint(NewTTP, TC, Args);
+SemaRef.SubstTypeConstraint(NewTTP, TC, Args, false);
   if (TTP->hasDefaultArgument()) {




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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

Rebased!


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

https://reviews.llvm.org/D126907

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/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/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/SemaTemplate/concepts-friends.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.

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

All tests pass now, I was able to get the template-template checks working 
correctly, and it passes all the tests I have available.  @ChuanqiXu if you 
could review/run what tests you have, it would be greatly appreciated.


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

https://reviews.llvm.org/D126907

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/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/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/SemaTemplate/concepts-friends.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] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked an inline comment as done.
erichkeane added a comment.

In D126907#3600876 , @ChuanqiXu wrote:

> Great progress!
>
> In D126907#3599835 , @erichkeane 
> wrote:
>
>> Note that the failure comes down to:
>>
>>   template concept C = T::f();
>>   template class P> struct S1{};
>>   template struct X{};
>>   S1 s11;
>>
>> and requires the -frelaxed-template-template-args flag:
>>
>>   [ekeane1@scsel-clx-24 build]$  ./bin/clang -cc1 -std=c++20 temp.cpp 
>> -frelaxed-template-template-args
>>   temp.cpp:5:4: error: template template argument 'X' is more constrained 
>> than template template parameter 'P'
>>   S1 s11;
>>  ^
>>   temp.cpp:3:29: note: 'P' declared here
>>   template  class P> struct S1{};
>>   ^
>>   temp.cpp:4:20: note: 'X' declared here
>>   template struct X{};
>>  ^
>>   1 error generated.
>
> As far as I could tell, we could omit the diagnostic by deleting 
> https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/include/clang/Sema/SemaConcept.h#L45-L53
>
> This change is obsolutely wrong but it shows the bug comes from 
> `ParameterMapping` so we could locate 
> https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/lib/Sema/SemaConcept.cpp#L723
>  further.

Thanks for the debugging help!  I'm not sure what you mean by "Locating it 
further"?  Best I can tell looking so far, is the fact that the 'depths' still 
point based on the 'root', but this is comparing thinking they are relative to 
the decl themselves.  I see the 'SubstTemplateArguments' there that looks 
suspicious, so there is probably something about setting up my template 
parameters correctly there/the MLTAL.

Thanks for the comments and debugging help again!




Comment at: clang/test/Driver/crash-report.cpp:28
+// RUNX: cat %t/crash-report-*.cpp | FileCheck --check-prefix=CHECKSRC %s
+// RUNX: cat %t/crash-report-*.sh | FileCheck --check-prefix=CHECKSH %s
 

ChuanqiXu wrote:
> What's the meaning of RUNX?
Woops!  I didn't add this test change to GIT, but it apparently still showed up 
with 'diff'.  For some reason, once I switched to lldb the llvm-symbolizer 
takes a HUGE amount of time during this test when doing check-all, so I used 
the 'X' to disable the test locally.  This is not intended to be committed.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Great progress!

In D126907#3599835 , @erichkeane 
wrote:

> Note that the failure comes down to:
>
>   template concept C = T::f();
>   template class P> struct S1{};
>   template struct X{};
>   S1 s11;
>
> and requires the -frelaxed-template-template-args flag:
>
>   [ekeane1@scsel-clx-24 build]$  ./bin/clang -cc1 -std=c++20 temp.cpp 
> -frelaxed-template-template-args
>   temp.cpp:5:4: error: template template argument 'X' is more constrained 
> than template template parameter 'P'
>   S1 s11;
>  ^
>   temp.cpp:3:29: note: 'P' declared here
>   template  class P> struct S1{};
>   ^
>   temp.cpp:4:20: note: 'X' declared here
>   template struct X{};
>  ^
>   1 error generated.

As far as I could tell, we could omit the diagnostic by deleting 
https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/include/clang/Sema/SemaConcept.h#L45-L53

This change is obsolutely wrong but it shows the bug comes from 
`ParameterMapping` so we could locate 
https://github.com/llvm/llvm-project/blob/bc74bca5363270e987c2e3c263bfaaeb6ceab66f/clang/lib/Sema/SemaConcept.cpp#L723
 further.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2461
 // substitution)
-if (SubstTypeConstraint(Inst, TC, TemplateArgs))
+if (SubstTypeConstraint(Inst, TC, TemplateArgs, false))
   return nullptr;





Comment at: clang/test/Driver/crash-report.cpp:28
+// RUNX: cat %t/crash-report-*.cpp | FileCheck --check-prefix=CHECKSRC %s
+// RUNX: cat %t/crash-report-*.sh | FileCheck --check-prefix=CHECKSH %s
 

What's the meaning of RUNX?


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Note that the failure comes down to:

  template concept C = T::f();
  template class P> struct S1{};
  template struct X{};
  S1 s11;

and requires the -frelaxed-template-template-args flag:

  [ekeane1@scsel-clx-24 build]$  ./bin/clang -cc1 -std=c++20 temp.cpp 
-frelaxed-template-template-args
  temp.cpp:5:4: error: template template argument 'X' is more constrained than 
template template parameter 'P'
  S1 s11;
 ^
  temp.cpp:3:29: note: 'P' declared here
  template  class P> struct S1{};
  ^
  temp.cpp:4:20: note: 'X' declared here
  template struct X{};
 ^
  1 error generated.




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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

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

As promised, got it down to the 1 failure, and added tests for @ChuanqiXu and 
the std::vector example.  That all seems to work, just a problem with the 
CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp test left.


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

https://reviews.llvm.org/D126907

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/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/CXX/temp/temp.constr/temp.constr.order/class-template-partial-specializations.cpp
  
clang/test/CXX/temp/temp.constr/temp.constr.order/var-template-partial-specializations.cpp
  clang/test/Driver/crash-report.cpp
  clang/test/SemaTemplate/concepts-friends.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), ...)
 // 

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Made good progress today, but got trapped down the CheckDeducedArgs path for 
quite a bit longer with partial specializations.  I have all of the crashes I 
know of 'fixed', but have a few tests that still fail for unknown reasons.  
Because of that, i don't have anything I can really upload, so I'll have to do 
so mid-next-week.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3591195 , @ChuanqiXu wrote:

> In D126907#3588708 , @erichkeane 
> wrote:
>
>> In D126907#3588417 , @ChuanqiXu 
>> wrote:
>>
>>> From what I can see, the crash reason would be the mismatch depth and the 
>>> setting of MultiLevelTemplateArgumentList. In 
>>> `::CheckConstraintSatisfaction`, the depth of `RawCompletionToken ` is 0, 
>>> while the depth of corresponding MultiLevelTemplateArgumentList is 2. So 
>>> the compiler would get the outermost template argument incorrectly (which 
>>> is a template pack in the example) in TransformTemplateTypeParmType.
>>>
>>> The first though was that we should set the depth of `RawCompletionToken ` 
>>> to 1 correctly. But I felt this might not be good later since in the normal 
>>> process of template instantiation (without concepts and constraints), the 
>>> depth of `RawCompletionToken ` is 0 indeed. What is different that time is 
>>> the depth of corresponding `MultiLevelTemplateArgumentList ` is 1. So it 
>>> looks like the process is constructed on the fly. It makes sense for the 
>>> perspective of compilation speed.
>>>
>>> So I feel like what we should do here is in 
>>> `Sema::CheckInstantiatedFunctionTemplateConstraints`, when we are computing 
>>> the desired MultiLevelTemplateArgumentList, we should compute a result with 
>>> depth of 1 in the particular case.
>>>
>>> ---
>>>
>>> Another idea is to not do instantiation when we're checking constraints. 
>>> But I need to think more about this.
>>
>> I don't know of the 'another idea'?  We have to do instantiation before 
>> checking, and if we do it too early, we aren't doing the deferred 
>> instantiation.  And the problem is that the RawCompletionToken uses a 
>> concept to refer to 'itself'.  However, this version of 'itself' isn't 
>> valid, since the 'depth' is wrong once it tries to instantiate relative to 
>> the 'top level'.  However, this IS happening during instantiation?
>>
>> My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT 
>> happen at the Sema level (but at the instantiator level), since it ends up 
>> catching things it shouldn't?  I tried it and have a handful of failures of 
>> trying to check uninstantiated constraints, but I've not dug completely into 
>> it.
>
> Yeah, we have to do instantiation before checking. My point is that it looks 
> like we're doing **another** instantiation when we check the concepts. And my 
> point is that if it we should avoid the second instantiation.

Hmm... yeah, I think we actually want to skip the FIRST instantiation?  I think 
the work I did above in Sema for `IsEvaluatingAConstraint` was too greedy, it 
ends up instantiating constraints on the 'while we are there' type things, 
rather than the things being currently checked.  I found that if I can make it 
a state of the instantiators themselves, it seems to work, at least for your 
example.  I've got it down to 1 'lit' test failure, but have been looking at 
the other problem first (below).

The Github-Issue crash IS a regression from this patch, and I've minimized it 
sufficiently.  I believe I have a hold on how to fix it, I just have to do so 
:)  If I make any further progress, I'll clean this up and put it up here, even 
if it DOES have the lit test failure.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D126907#3588708 , @erichkeane 
wrote:

> In D126907#3588417 , @ChuanqiXu 
> wrote:
>
>> From what I can see, the crash reason would be the mismatch depth and the 
>> setting of MultiLevelTemplateArgumentList. In 
>> `::CheckConstraintSatisfaction`, the depth of `RawCompletionToken ` is 0, 
>> while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the 
>> compiler would get the outermost template argument incorrectly (which is a 
>> template pack in the example) in TransformTemplateTypeParmType.
>>
>> The first though was that we should set the depth of `RawCompletionToken ` 
>> to 1 correctly. But I felt this might not be good later since in the normal 
>> process of template instantiation (without concepts and constraints), the 
>> depth of `RawCompletionToken ` is 0 indeed. What is different that time is 
>> the depth of corresponding `MultiLevelTemplateArgumentList ` is 1. So it 
>> looks like the process is constructed on the fly. It makes sense for the 
>> perspective of compilation speed.
>>
>> So I feel like what we should do here is in 
>> `Sema::CheckInstantiatedFunctionTemplateConstraints`, when we are computing 
>> the desired MultiLevelTemplateArgumentList, we should compute a result with 
>> depth of 1 in the particular case.
>>
>> ---
>>
>> Another idea is to not do instantiation when we're checking constraints. But 
>> I need to think more about this.
>
> I don't know of the 'another idea'?  We have to do instantiation before 
> checking, and if we do it too early, we aren't doing the deferred 
> instantiation.  And the problem is that the RawCompletionToken uses a concept 
> to refer to 'itself'.  However, this version of 'itself' isn't valid, since 
> the 'depth' is wrong once it tries to instantiate relative to the 'top 
> level'.  However, this IS happening during instantiation?
>
> My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT 
> happen at the Sema level (but at the instantiator level), since it ends up 
> catching things it shouldn't?  I tried it and have a handful of failures of 
> trying to check uninstantiated constraints, but I've not dug completely into 
> it.

Yeah, we have to do instantiation before checking. My point is that it looks 
like we're doing **another** instantiation when we check the concepts. And my 
point is that if it we should avoid the second instantiation.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3588417 , @ChuanqiXu wrote:

> From what I can see, the crash reason would be the mismatch depth and the 
> setting of MultiLevelTemplateArgumentList. In 
> `::CheckConstraintSatisfaction`, the depth of `RawCompletionToken ` is 0, 
> while the depth of corresponding MultiLevelTemplateArgumentList is 2. So the 
> compiler would get the outermost template argument incorrectly (which is a 
> template pack in the example) in TransformTemplateTypeParmType.
>
> The first though was that we should set the depth of `RawCompletionToken ` to 
> 1 correctly. But I felt this might not be good later since in the normal 
> process of template instantiation (without concepts and constraints), the 
> depth of `RawCompletionToken ` is 0 indeed. What is different that time is 
> the depth of corresponding `MultiLevelTemplateArgumentList ` is 1. So it 
> looks like the process is constructed on the fly. It makes sense for the 
> perspective of compilation speed.
>
> So I feel like what we should do here is in 
> `Sema::CheckInstantiatedFunctionTemplateConstraints`, when we are computing 
> the desired MultiLevelTemplateArgumentList, we should compute a result with 
> depth of 1 in the particular case.
>
> ---
>
> Another idea is to not do instantiation when we're checking constraints. But 
> I need to think more about this.

I don't know of the 'another idea'?  We have to do instantiation before 
checking, and if we do it too early, we aren't doing the deferred 
instantiation.  And the problem is that the RawCompletionToken uses a concept 
to refer to 'itself'.  However, this version of 'itself' isn't valid, since the 
'depth' is wrong once it tries to instantiate relative to the 'top level'.  
However, this IS happening during instantiation?

My latest thought is that perhaps the "IsEvaluatingAConstraint" should NOT 
happen at the Sema level (but at the instantiator level), since it ends up 
catching things it shouldn't?  I tried it and have a handful of failures of 
trying to check uninstantiated constraints, but I've not dug completely into it.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

From what I can see, the crash reason would be the mismatch depth and the 
setting of MultiLevelTemplateArgumentList. In `::CheckConstraintSatisfaction`, 
the depth of `RawCompletionToken ` is 0, while the depth of corresponding 
MultiLevelTemplateArgumentList is 2. So the compiler would get the outermost 
template argument incorrectly (which is a template pack in the example) in 
TransformTemplateTypeParmType.

The first though was that we should set the depth of `RawCompletionToken ` to 1 
correctly. But I felt this might not be good later since in the normal process 
of template instantiation (without concepts and constraints), the depth of 
`RawCompletionToken ` is 0 indeed. What is different that time is the depth of 
corresponding `MultiLevelTemplateArgumentList ` is 1. So it looks like the 
process is constructed on the fly. It makes sense for the perspective of 
compilation speed.

So I feel like what we should do here is in 
`Sema::CheckInstantiatedFunctionTemplateConstraints`, when we are computing the 
desired MultiLevelTemplateArgumentList, we should compute a result with depth 
of 1 in the particular case.

---

Another idea is to not do instantiation when we're checking constraints. But I 
need to think more about this.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3585626 , @erichkeane 
wrote:

> See here: https://github.com/llvm/llvm-project/issues/55673
>
> This might be the same issue, the crash at least looks the same.

Note: Looks to not be the same problem :/  Additionally, the fix I thought 
would work here does not, so I'm back at square 1 on that crash again.  I was 
trying to move the 'isEvaluatingAConstraint' out of Sema (since we don't want 
to isntantiate a type constraint until we are checking it!) but that causes us 
to fail a number of OTHER tests for checking an uninstantiated constraint.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

See here: https://github.com/llvm/llvm-project/issues/55673

This might be the same issue, the crash at least looks the same.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D126907#3584555 , @ChuanqiXu wrote:

> I finally found some time to look at the crash. Although I haven't get an 
> idea, I found it crash at the following one too:
>
>   template 
>   concept Constraint = true;
>   
>   template 
>   class completion_handler_async_result {
>   public:
>   template 
>   static void initiate(Initiation&& initiation, RawCompletionToken&& 
> token);
>   };
>   
>   template 
>   concept Constraint2 =
>   requires() {
>   completion_handler_async_result::initiate(0, 0);
>   };
>   
>   template 
>   void use(int F)
>   requires Constraint2
>   {}
>
> The crash log is:
>
>   clang/lib/AST/ExprConstant.cpp:15083: bool 
> clang::Expr::EvaluateAsConstantExpr(clang::Expr::EvalResult&, const 
> clang::ASTContext&, clang::Expr::ConstantExprKind) const: Assertion 
> `!isValueDependent() && "Expression evaluator can't be called on a dependent 
> expression."' failed.
>
> I am not sure if they are the same problem.

Yep, that is a different crash for the same reason (mismatched 'depth' of the 
template parameters).  I saw that a few times as well, but I believe anything 
we do to solve the first will solve this as well.  I haven't had a chance all 
week to get back to this, but hopefully will be able to work more on it by EOW.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I finally found some time to look at the crash. Although I haven't get an idea, 
I found it crash at the following one too:

  template 
  concept Constraint = true;
  
  template 
  class completion_handler_async_result {
  public:
  template 
  static void initiate(Initiation&& initiation, RawCompletionToken&& token);
  };
  
  template 
  concept Constraint2 =
  requires(float&& t) {
  completion_handler_async_result::initiate(0, 0);
  };
  
  template 
  void use(int F)
  requires Constraint2
  {}

I am not sure if they are the same problem.


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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So from what I can tell, the problem is evaluating `Constraint` on `initiate`, 
and trying to find the 'right' argument from the 
`MultiLevelTemplateArgumentList` for `RawCompletionToken`.  The problem is that 
the `RawCompletionToken`, as a template parameter, gets 'filled in to' the 
Concept relative to `initiate` instead of relative to the "root".

I suspect we have to go back to when the `ConceptSpecializationExpr` was 
created, and change the template argument to be relative to the top?  I'm not 
sure that is the right answer here, or what else that could screw up?  In my 
head, I think it should be 'fine', since the 'relative to the top' is the way 
we evaluate ALL constraints (including these in the template argument), but I 
probably need to think it through.

See:

  FunctionTemplateDecl 0x13575598  
col:17 initiate
  |-TemplateTypeParmDecl 0x13573fd8  col:24 typename depth 0 
index 0 Initiation
  |-TemplateTypeParmDecl 0x13574060  col:47 Concept 0x13555838 
'Constraint' depth 0 index 1 RawCompletionToken
  | `-ConceptSpecializationExpr 0x135741a8  'bool' Concept 0x13555838 
'Constraint'
  |   `-TemplateArgument  type 'RawCompletionToken'
  | `-TemplateTypeParmType 0x13574130 'RawCompletionToken' dependent depth 
0 index 1
  |   `-TemplateTypeParm 0x13574060 'RawCompletionToken'




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

https://reviews.llvm.org/D126907

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