subject:"\[PATCH\] D146178\: \[Clang\]\[Sema\] Fix comparison of constraint expressions"

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-07-12 Thread Shafik Yaghmour via Phabricator via cfe-commits

shafik added a comment.

Looks like this bug is caused by this commit: 
https://github.com/llvm/llvm-project/issues/63782#issuecomment-1633312909


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-15 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@erichkeane - I took your example and tried to reduce it further
https://godbolt.org/z/jEx9vdj7K

It's kind of a difficult situation - both gcc and msvc accept it, yet /* very 
very cautiously */ it might happen that the code is actually invalid ...
(i'd need to think about it more), but this is based on the insights from 
@rsmith  and http://eel.is/c++draft/temp.constr#atomic-3.sentence-5 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-15 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

There is probably more reduction work to be done, but I hit the end of my day 
here:

https://godbolt.org/z/Tzfx31asK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-15 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D146178#4343477 , 
@alexander-shaposhnikov wrote:

> @erichkeane - I'll have stable internet ~soon and will try to look into the 
> reported issue (but help would be greatly appreciated).
> To the best of my knowledge there are other problems with libstdc++'s ranges 
> (even without this diff), but yeah, this regression is unfortunate.

I've been working on a reduction most of the day, I'll share it when I get it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-15 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@erichkeane - I'll have stable internet ~soon and will try to look into the 
reported issue (but help would be greatly appreciated).
To the best of my knowledge there are other problems with libstdc++'s ranges 
(even without this diff), but yeah, this regression is unfortunate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-15 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D146178#4339561 , @awson wrote:

> Now, this
>
>   #include 
>   #include 
>   
>   auto drop1(const std::vector& s){
>   return s | std::views::drop(1);
>   }
>
> when compiled against gcc's-13.1 libstdc++ spits:
>
>   boro.cpp:5:11: error: invalid operands to binary expression ('const 
> std::vector' and '_Partial<_Drop, decay_t>' (aka 
> '_Partial'))
>   return s | std::views::drop(1);
>  ~ ^ ~~~
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\cstddef:135:3: note: candidate 
> function not viable: no known conversion from 'const std::vector' to 
> 'byte' for 1st argument
> operator|(byte __l, byte __r) noexcept
> ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\bits/ios_base.h:87:3: note: 
> candidate function not viable: no known conversion from 'const 
> std::vector' to '_Ios_Fmtflags' for 1st argument
> operator|(_Ios_Fmtflags __a, _Ios_Fmtflags __b)
> ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\bits/ios_base.h:130:3: note: 
> candidate function not viable: no known conversion from 'const 
> std::vector' to '_Ios_Openmode' for 1st argument
> operator|(_Ios_Openmode __a, _Ios_Openmode __b)
> ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\bits/ios_base.h:170:3: note: 
> candidate function not viable: no known conversion from 'const 
> std::vector' to '_Ios_Iostate' for 1st argument
> operator|(_Ios_Iostate __a, _Ios_Iostate __b)
> ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:887:7: note: candidate 
> template ignored: constraints not satisfied [with _Self = _Partial<_Drop, 
> decay_t>, _Range = const std::vector &]
> operator|(_Range&& __r, _Self&& __self)
> ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:885:5: note: because 
> '__adaptor_invocable  int>, const std::vector &>' evaluated to false
>   && __adaptor_invocable<_Self, _Range>
>  ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:860:20: note: because 
> 'std::declval<_Adaptor>()(declval<_Args>()...)' would be invalid: no matching 
> function for call to object of type 
> 'std::ranges::views::__adaptor::_Partial'
> = requires { std::declval<_Adaptor>()(declval<_Args>()...); };
>  ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:896:7: note: candidate 
> template ignored: constraints not satisfied [with _Lhs = std::vector, 
> _Rhs = _Partial<_Drop, decay_t>]
> operator|(_Lhs __lhs, _Rhs __rhs)
> ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:893:16: note: because 
> 'derived_from, _RangeAdaptorClosure>' evaluated to false
> requires derived_from<_Lhs, _RangeAdaptorClosure>
>  ^
>   C:\Progs\msys64\ucrt64\include\c++\13.1.0\concepts:67:28: note: because 
> '__is_base_of(std::ranges::views::__adaptor::_RangeAdaptorClosure, 
> std::vector)' evaluated to false
>   concept derived_from = __is_base_of(_Base, _Derived)
>  ^
>   1 error generated.
>
> `transform` doesn't work either.

It isn't clear to me what the issue is here from looking at that.  
@alexander-shaposhnikov : Any chance you have time soon to analyze this so we 
don't have to revert?  I'd very much like to not have to do that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-12 Thread Kyrill Briantsev via Phabricator via cfe-commits

awson added a comment.

Now, this

  #include 
  #include 
  
  auto drop1(const std::vector& s){
return s | std::views::drop(1);
  }

when compiled against gcc's-13.1 libstdc++ spits:

  boro.cpp:5:11: error: invalid operands to binary expression ('const 
std::vector' and '_Partial<_Drop, decay_t>' (aka 
'_Partial'))
  return s | std::views::drop(1);
 ~ ^ ~~~
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\cstddef:135:3: note: candidate 
function not viable: no known conversion from 'const std::vector' to 
'byte' for 1st argument
operator|(byte __l, byte __r) noexcept
^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\bits/ios_base.h:87:3: note: 
candidate function not viable: no known conversion from 'const 
std::vector' to '_Ios_Fmtflags' for 1st argument
operator|(_Ios_Fmtflags __a, _Ios_Fmtflags __b)
^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\bits/ios_base.h:130:3: note: 
candidate function not viable: no known conversion from 'const 
std::vector' to '_Ios_Openmode' for 1st argument
operator|(_Ios_Openmode __a, _Ios_Openmode __b)
^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\bits/ios_base.h:170:3: note: 
candidate function not viable: no known conversion from 'const 
std::vector' to '_Ios_Iostate' for 1st argument
operator|(_Ios_Iostate __a, _Ios_Iostate __b)
^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:887:7: note: candidate 
template ignored: constraints not satisfied [with _Self = _Partial<_Drop, 
decay_t>, _Range = const std::vector &]
operator|(_Range&& __r, _Self&& __self)
^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:885:5: note: because 
'__adaptor_invocable, const std::vector &>' evaluated to false
  && __adaptor_invocable<_Self, _Range>
 ^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:860:20: note: because 
'std::declval<_Adaptor>()(declval<_Args>()...)' would be invalid: no matching 
function for call to object of type 
'std::ranges::views::__adaptor::_Partial'
= requires { std::declval<_Adaptor>()(declval<_Args>()...); };
 ^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:896:7: note: candidate 
template ignored: constraints not satisfied [with _Lhs = std::vector, _Rhs 
= _Partial<_Drop, decay_t>]
operator|(_Lhs __lhs, _Rhs __rhs)
^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\ranges:893:16: note: because 
'derived_from, _RangeAdaptorClosure>' evaluated to false
requires derived_from<_Lhs, _RangeAdaptorClosure>
 ^
  C:\Progs\msys64\ucrt64\include\c++\13.1.0\concepts:67:28: note: because 
'__is_base_of(std::ranges::views::__adaptor::_RangeAdaptorClosure, 
std::vector)' evaluated to false
  concept derived_from = __is_base_of(_Base, _Derived)
 ^
  1 error generated.

`transform` doesn't work either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-05 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

I see this is closed... but I accept if you wish to re-commit this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-05 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:133
 }
 
+Response HandlePartialClassTemplateSpec(

alexander-shaposhnikov wrote:
> alexander-shaposhnikov wrote:
> > alexander-shaposhnikov wrote:
> > > alexander-shaposhnikov wrote:
> > > > HandlePartialClassTemplateSpec is from Erich's diff 
> > > > (https://reviews.llvm.org/D147722)
> > > @erichkeane : previously (see https://reviews.llvm.org/D147722) we were 
> > > producing a real layer of arguments (if the depth is greater than 1), to 
> > > the best of my knowledge this is incorrect since it'd trigger unexpected 
> > > depth adjustment, we should produce retained layers instead.
> > (depth adjustment during the substitution)
> to make it a bit easier to reason about I've changed the names in the test 
> case:
> 
> ```
> //enum class Enum { E1 };
> 
> template 
> concept some_concept = true;
> //inline constexpr bool some_concept = true;
> 
> template 
> struct S {
>   template 
>   requires(some_concept)
>   void func(const T2 &);
> };
> 
> template 
> struct S {
>   template 
>   requires(some_concept)
>   void func(const T22 &);
> };
> 
> template 
> template 
> requires (some_concept)
> inline void S::func(const T222 &) {}
> ```
> 
> Previously the code was doing the following:
> ```
> PartialClassTemplSpec->getTemplateDepth() = 1
> NumRetainedOuterLevels: 0
> 0: <>
> before subst:
> ParenExpr 0x558fb90be6a8 '_Bool'
> `-ConceptSpecializationExpr 0x558fb90be640 '_Bool' Concept 0x558fb909fbd8 
> 'some_concept'
>   |-ImplicitConceptSpecializationDecl 0x558fb90be5d0 
>   | `-TemplateArgument type 'type-parameter-1-0'
>   |   `-TemplateTypeParmType 0x558fb90a01d0 'type-parameter-1-0' dependent 
> depth 1 index 0
>   `-TemplateArgument type 'T22'
> `-TemplateTypeParmType 0x558fb90be590 'T22' dependent depth 1 index 0
>   `-TemplateTypeParm 0x558fb90be540 'T22'
> after subst:
> ParenExpr 0x558fb90bf168 '_Bool'
> `-ConceptSpecializationExpr 0x558fb90bf100 '_Bool' Concept 0x558fb909fbd8 
> 'some_concept'
>   |-ImplicitConceptSpecializationDecl 0x558fb90bf090 
>   | `-TemplateArgument type 'type-parameter-0-0'
>   |   `-TemplateTypeParmType 0x558fb909fb50 'type-parameter-0-0' dependent 
> depth 0 index 0
>   `-TemplateArgument type 'T22'
> `-TemplateTypeParmType 0x558fb90bf050 'T22' dependent depth 0 index 0
>   `-TemplateTypeParm 0x558fb90be540 'T22'
> ```
> (as one can see the depths have changed)
> 
> now:
> ```
> PartialClassTemplSpec->getTemplateDepth() = 1
> NumRetainedOuterLevels: 1
> before subst:
> ParenExpr 0x55f7813ae6a8 '_Bool'
> `-ConceptSpecializationExpr 0x55f7813ae640 '_Bool' Concept 0x55f78138fbd8 
> 'some_concept'
>   |-ImplicitConceptSpecializationDecl 0x55f7813ae5d0 
>   | `-TemplateArgument type 'type-parameter-1-0'
>   |   `-TemplateTypeParmType 0x55f7813901d0 'type-parameter-1-0' dependent 
> depth 1 index 0
>   `-TemplateArgument type 'T22'
> `-TemplateTypeParmType 0x55f7813ae590 'T22' dependent depth 1 index 0
>   `-TemplateTypeParm 0x55f7813ae540 'T22'
> after subst:
> ParenExpr 0x55f7813af138 '_Bool'
> `-ConceptSpecializationExpr 0x55f7813af0d0 '_Bool' Concept 0x55f78138fbd8 
> 'some_concept'
>   |-ImplicitConceptSpecializationDecl 0x55f7813af060 
>   | `-TemplateArgument type 'type-parameter-1-0'
>   |   `-TemplateTypeParmType 0x55f7813901d0 'type-parameter-1-0' dependent 
> depth 1 index 0
>   `-TemplateArgument type 'T22'
> `-TemplateTypeParmType 0x55f7813ae590 'T22' dependent depth 1 index 0
>   `-TemplateTypeParm 0x55f7813ae540 'T22'
> ```
> 
> 
So the thought behind the 'empty' level was for a case where the 
`PartialClassTemplSpec` is defined inside of another type that actually DOES 
have arguments we need to compare.  Though, I guess for the purposes of 
substitution, it isn't really needed (since if we have a 
`PartialClassTemplSpec`, we can't fully instantiate ANYWAY)... and I can't 
think of a case where it totally matters anyway.  I think I'm OK with this as 
is, but I'm a touch suspicious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-05 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:133
 }
 
+Response HandlePartialClassTemplateSpec(

alexander-shaposhnikov wrote:
> alexander-shaposhnikov wrote:
> > alexander-shaposhnikov wrote:
> > > HandlePartialClassTemplateSpec is from Erich's diff 
> > > (https://reviews.llvm.org/D147722)
> > @erichkeane : previously (see https://reviews.llvm.org/D147722) we were 
> > producing a real layer of arguments (if the depth is greater than 1), to 
> > the best of my knowledge this is incorrect since it'd trigger unexpected 
> > depth adjustment, we should produce retained layers instead.
> (depth adjustment during the substitution)
to make it a bit easier to reason about I've changed the names in the test case:

```
//enum class Enum { E1 };

template 
concept some_concept = true;
//inline constexpr bool some_concept = true;

template 
struct S {
  template 
  requires(some_concept)
  void func(const T2 &);
};

template 
struct S {
  template 
  requires(some_concept)
  void func(const T22 &);
};

template 
template 
requires (some_concept)
inline void S::func(const T222 &) {}
```

Previously the code was doing the following:
```
PartialClassTemplSpec->getTemplateDepth() = 1
NumRetainedOuterLevels: 0
0: <>
before subst:
ParenExpr 0x558fb90be6a8 '_Bool'
`-ConceptSpecializationExpr 0x558fb90be640 '_Bool' Concept 0x558fb909fbd8 
'some_concept'
  |-ImplicitConceptSpecializationDecl 0x558fb90be5d0 
  | `-TemplateArgument type 'type-parameter-1-0'
  |   `-TemplateTypeParmType 0x558fb90a01d0 'type-parameter-1-0' dependent 
depth 1 index 0
  `-TemplateArgument type 'T22'
`-TemplateTypeParmType 0x558fb90be590 'T22' dependent depth 1 index 0
  `-TemplateTypeParm 0x558fb90be540 'T22'
after subst:
ParenExpr 0x558fb90bf168 '_Bool'
`-ConceptSpecializationExpr 0x558fb90bf100 '_Bool' Concept 0x558fb909fbd8 
'some_concept'
  |-ImplicitConceptSpecializationDecl 0x558fb90bf090 
  | `-TemplateArgument type 'type-parameter-0-0'
  |   `-TemplateTypeParmType 0x558fb909fb50 'type-parameter-0-0' dependent 
depth 0 index 0
  `-TemplateArgument type 'T22'
`-TemplateTypeParmType 0x558fb90bf050 'T22' dependent depth 0 index 0
  `-TemplateTypeParm 0x558fb90be540 'T22'
```
(as one can see the depths have changed)

now:
```
PartialClassTemplSpec->getTemplateDepth() = 1
NumRetainedOuterLevels: 1
before subst:
ParenExpr 0x55f7813ae6a8 '_Bool'
`-ConceptSpecializationExpr 0x55f7813ae640 '_Bool' Concept 0x55f78138fbd8 
'some_concept'
  |-ImplicitConceptSpecializationDecl 0x55f7813ae5d0 
  | `-TemplateArgument type 'type-parameter-1-0'
  |   `-TemplateTypeParmType 0x55f7813901d0 'type-parameter-1-0' dependent 
depth 1 index 0
  `-TemplateArgument type 'T22'
`-TemplateTypeParmType 0x55f7813ae590 'T22' dependent depth 1 index 0
  `-TemplateTypeParm 0x55f7813ae540 'T22'
after subst:
ParenExpr 0x55f7813af138 '_Bool'
`-ConceptSpecializationExpr 0x55f7813af0d0 '_Bool' Concept 0x55f78138fbd8 
'some_concept'
  |-ImplicitConceptSpecializationDecl 0x55f7813af060 
  | `-TemplateArgument type 'type-parameter-1-0'
  |   `-TemplateTypeParmType 0x55f7813901d0 'type-parameter-1-0' dependent 
depth 1 index 0
  `-TemplateArgument type 'T22'
`-TemplateTypeParmType 0x55f7813ae590 'T22' dependent depth 1 index 0
  `-TemplateTypeParm 0x55f7813ae540 'T22'
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-04 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:133
 }
 
+Response HandlePartialClassTemplateSpec(

alexander-shaposhnikov wrote:
> alexander-shaposhnikov wrote:
> > HandlePartialClassTemplateSpec is from Erich's diff 
> > (https://reviews.llvm.org/D147722)
> @erichkeane : previously (see https://reviews.llvm.org/D147722) we were 
> producing a real layer of arguments (if the depth is greater than 1), to the 
> best of my knowledge this is incorrect since it'd trigger unexpected depth 
> adjustment, we should produce retained layers instead.
(depth adjustment during the substitution)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-04 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@davidtgoldblatt - thanks for the report.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-04 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/test/SemaTemplate/concepts-out-of-line-def.cpp:348
+
+namespace MultilevelTemplateWithPartialSpecialization {
+template 

(new tests)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-04 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:133
 }
 
+Response HandlePartialClassTemplateSpec(

alexander-shaposhnikov wrote:
> HandlePartialClassTemplateSpec is from Erich's diff 
> (https://reviews.llvm.org/D147722)
@erichkeane : previously (see https://reviews.llvm.org/D147722) we were 
producing a real layer of arguments (if the depth is greater than 1), to the 
best of my knowledge this is incorrect since it'd trigger unexpected depth 
adjustment, we should produce retained layers instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-04 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 519711.
alexander-shaposhnikov added a comment.

Remove dead code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/AST/DeclTemplate.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-friends.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,101 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+
+template 
+concept C1 = false;
+
+struct Foo {
+  template 
+  struct Bar {};
+
+  template 
+requires(C1)
+  struct Bar;
+};
+
+Foo::Bar BarInstance;
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+} // namespace GH61959
+
+
+namespace TemplateInsideTemplateInsideTemplate {
+template
+concept C1 = false;
+
+template 
+struct W0 {
+  template 
+  struct W1 {
+template 
+struct F {
+  enum { value = 1 };
+};
+
+template 
+  requires C1
+struct F {
+  enum { value = 2 };
+};
+  };
+};
+
+static_assert(W0<0>::W1<1>::F::value == 1);
+} // TemplateInsideTemplateInsideTemplate
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,275 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-04 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 519708.
alexander-shaposhnikov added a comment.

Add more tests,
Fix HandlePartialClassTemplateSpec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/AST/DeclTemplate.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-friends.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,101 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+
+template 
+concept C1 = false;
+
+struct Foo {
+  template 
+  struct Bar {};
+
+  template 
+requires(C1)
+  struct Bar;
+};
+
+Foo::Bar BarInstance;
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+} // namespace GH61959
+
+
+namespace TemplateInsideTemplateInsideTemplate {
+template
+concept C1 = false;
+
+template 
+struct W0 {
+  template 
+  struct W1 {
+template 
+struct F {
+  enum { value = 1 };
+};
+
+template 
+  requires C1
+struct F {
+  enum { value = 2 };
+};
+  };
+};
+
+static_assert(W0<0>::W1<1>::F::value == 1);
+} // TemplateInsideTemplateInsideTemplate
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,275 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() {

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-04 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

Reverted in 
https://github.com/llvm/llvm-project/commit/3b9ed6e5323176550925f3b0a2c50ced1b61438d,
it'll take time to investigate this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-04 Thread David Goldblatt via Phabricator via cfe-commits

davidtgoldblatt added a comment.

This version of the commit also introduces some breakages; as before I'm not 
sure if it's the code or the diff that's incorrect.

Repro:

  enum class Enum { E1 };
  
  template 
  inline constexpr bool some_concept = true;
  
  template 
  struct S {
template 
requires some_concept
void func(const T2 &);
  };
  
  template 
  struct S {
template 
requires some_concept
void func(const T2 &);
  };
  
  template 
  template 
  requires some_concept
  inline void S::func(const T2 &) {}

Error:

  repro.cpp:23:30: error: out-of-line definition of 'func' does not match any 
declaration in 'S'
  inline void S::func(const T2 &) {}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-03 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

P.S. Landed. If it survives in the trunk this time - I'll send a follow-up diff 
with the release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-03 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@erichkeane - thanks, then I'm going to give it another try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-03 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

Missing context in the diff, but the changes since commit are all sensible to 
me.  If this fixes everything we know about, I'm OK with it.




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:344
  Innermost->asArray(), Final);
-
-  const Decl *CurDecl = ND;
+  CurDecl = Response::UseNextDecl(ND).NextDecl;
+  }

alexander-shaposhnikov wrote:
> this bit is important.
Oh yeah, that makes a lot of sense.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-03 Thread Stephan Bergmann via Phabricator via cfe-commits

sberg added a comment.

In D146178#4314999 , @sberg wrote:

> Since this commit...

ah, already taken care of with 
https://github.com/llvm/llvm-project/commit/3e850a6eea5277082a0b7b701754c86530d25c40
 "Revert '[Clang][Sema] Fix comparison of constraint expressions'"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-03 Thread Stephan Bergmann via Phabricator via cfe-commits

sberg added a comment.

Since this commit (plus its follow-up 
https://github.com/llvm/llvm-project/commit/ce861ec782ae3f41807b61e855512aaccf3c2149
 "[Clang][Sema] Add a temporary workaround in SemaConcept.cpp", to avoid 
`clang/lib/AST/ExprConstant.cpp:15332: bool 
clang::Expr::EvaluateAsConstantExpr(EvalResult &, const ASTContext &, 
ConstantExprKind) const: Assertion `!isValueDependent() && "Expression 
evaluator can't be called on a dependent expression."' failed` in 
`-DLLVM_ENABLE_ASSERTIONS=ON` builds):

  $ cat test.cc
  #include 
  #include 
  #include 
  std::vector> v1;
  std::vector> v2;
  void f() {
v2.insert(v2.end(), std::make_move_iterator(v1.begin()), 
std::make_move_iterator(v1.end()));
  }

  $ clang++ -std=c++20 -fsyntax-only test.cc
  In file included from test.cc:1:
  In file included from 
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/memory:66:
  In file included from 
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_tempbuf.h:61:
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_construct.h:115:4:
 error: no matching function for call to 'construct_at'
std::construct_at(__p, std::forward<_Args>(__args)...);
^
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_uninitialized.h:120:11:
 note: in instantiation of function template specialization 
'std::_Construct, std::unique_ptr &>' requested here
  std::_Construct(std::__addressof(*__cur), *__first);
   ^
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_uninitialized.h:371:14:
 note: in instantiation of function template specialization 
'std::__do_uninit_copy *>, 
std::unique_ptr *>' requested here
  return std::__do_uninit_copy(__first, __last, __result);
  ^
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_uninitialized.h:384:19:
 note: in instantiation of function template specialization 
'std::__uninitialized_copy_a *>, 
std::unique_ptr *, std::unique_ptr>' requested here
return std::__uninitialized_copy_a(_GLIBCXX_MAKE_MOVE_ITERATOR(__first),
^
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/vector.tcc:766:12:
 note: in instantiation of function template specialization 
'std::__uninitialized_move_a *, std::unique_ptr *, 
std::allocator>>' requested here
  std::__uninitialized_move_a(this->_M_impl._M_finish - __n,
   ^
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_vector.h:1481:4:
 note: in instantiation of function template specialization 
'std::vector>::_M_range_insert
 *, std::vector' requested here
_M_range_insert(begin() + __offset, __first, __last,
^
  test.cc:7:6: note: in instantiation of function template specialization 
'std::vector>::insert
 *, std::vector>>>, void>' requested here
v2.insert(v2.end(), std::make_move_iterator(v1.begin()), 
std::make_move_iterator(v1.end()));
   ^
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_construct.h:94:5:
 note: candidate template ignored: substitution failure [with _Tp = 
std::unique_ptr, _Args =  &>]: call to deleted 
constructor of 'std::unique_ptr'
  construct_at(_Tp* __location, _Args&&... __args)
  ^
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_construct.h:119:25:
 error: call to deleted constructor of 'std::unique_ptr'
::new((void*)__p) _Tp(std::forward<_Args>(__args)...);
  ^   ~~~
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/unique_ptr.h:522:7:
 note: 'unique_ptr' has been explicitly marked deleted here
unique_ptr(const unique_ptr&) = delete;
^
  In file included from test.cc:1:
  In file included from 
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/memory:69:
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_uninitialized.h:120:6:
 error: no matching function for call to '_Construct'
  std::_Construct(std::__addressof(*__cur), *__first);
  ^~~
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stl_uninitialized.h:371:14:
 note: in instantiation of function template specialization 
'std::__do_uninit_copy
 *, std::vector>>>, std::unique_ptr *>' requested here
  return std::__do_uninit_copy(__first, __last, __result);
  ^
  
/usr/lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/vector.tcc:781:12:
 note: in instantiation of function template specialization 
'std::__uninitialized_copy_a
 *, std::vector>>>, std::unique_ptr *, 
std::unique_ptr>' requested here
  std::__uninitialized_copy_a(__mid, __last,
   ^

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-03 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:133
 }
 
+Response HandlePartialClassTemplateSpec(

HandlePartialClassTemplateSpec is from Erich's diff 
(https://reviews.llvm.org/D147722)



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:344
  Innermost->asArray(), Final);
-
-  const Decl *CurDecl = ND;
+  CurDecl = Response::UseNextDecl(ND).NextDecl;
+  }

this bit is important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-03 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 518990.
alexander-shaposhnikov added a comment.

Simplify code a little bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/AST/DeclTemplate.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-friends.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,101 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+
+template 
+concept C1 = false;
+
+struct Foo {
+  template 
+  struct Bar {};
+
+  template 
+requires(C1)
+  struct Bar;
+};
+
+Foo::Bar BarInstance;
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+} // namespace GH61959
+
+
+namespace TemplateInsideTemplateInsideTemplate {
+template
+concept C1 = false;
+
+template 
+struct W0 {
+  template 
+  struct W1 {
+template 
+struct F {
+  enum { value = 1 };
+};
+
+template 
+  requires C1
+struct F {
+  enum { value = 2 };
+};
+  };
+};
+
+static_assert(W0<0>::W1<1>::F::value == 1);
+} // TemplateInsideTemplateInsideTemplate
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,220 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-02 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 518966.
alexander-shaposhnikov added a comment.

1. If innermost != nullptr (in getTemplateInstantiationArgs) and NS is a 
ClassTemplatePartialSpecializationDecl we were incorrectly adding the inner 
level of template args twice (once as an array of arguments and once as a 
retained level inside HandlePartialClassTemplateSpec )
2. Add more tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/AST/DeclTemplate.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-friends.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,101 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+
+template 
+concept C1 = false;
+
+struct Foo {
+  template 
+  struct Bar {};
+
+  template 
+requires(C1)
+  struct Bar;
+};
+
+Foo::Bar BarInstance;
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+} // namespace GH61959
+
+
+namespace TemplateInsideTemplateInsideTemplate {
+template
+concept C1 = false;
+
+template 
+struct W0 {
+  template 
+  struct W1 {
+template 
+struct F {
+  enum { value = 1 };
+};
+
+template 
+  requires C1
+struct F {
+  enum { value = 2 };
+};
+  };
+};
+
+static_assert(W0<0>::W1<1>::F::value == 1);
+} // TemplateInsideTemplateInsideTemplate
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,220 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-02 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

upd.
In the reduced example above MLTAL is incorrect

  (lldb) p MLTAL.dump()
  NumRetainedOuterLevels: 1
  1: 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-02 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

reduced test case:

  template 
  concept Concept = false;
  
  struct Foo {
template 
struct result {};
  
template 
  requires(Concept<_Tp>)
struct result<_Tp>;
  };


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-02 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D146178#4313058 , 
@alexander-shaposhnikov wrote:

> @erichkeane  - feel free to take over this patch.

If I get time, I will!  Else it'll be here when you get back :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-02 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@erichkeane  - feel free to take over this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-02 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

I reverted due to the workaround (which I'd objected to before, I don't think 
it is acceptable) and the breakage David mentioned.  I'll make sure to review 
the commit message when you put it back up for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-02 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D146178#4312473 , @davidtgoldblatt 
wrote:

> This breaks ~the world in the versions of libstdc++ I can easily check -- see 
> e.g. https://gcc.godbolt.org/z/ETeGzc3ve (crashes at this commit, changes to 
> an error at ce861ec782ae3f41807b61e855512aaccf3c2149 
> ).
>
> I'm not deep enough into this nook of the language to tell if the bug is here 
> or libstdc++, though.

Anything that is fixed by e861ec782 is almost definitely a clang bug. I 
objected to that quite strongly in this patch, and am quite disappointed that 
it made it back in.  We're likely going to have a significant number of clang 
bugs that were previously an assert now be an odd, incorrect diagnostic.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-02 Thread David Goldblatt via Phabricator via cfe-commits

davidtgoldblatt added a comment.

This breaks ~the world in the versions of libstdc++ I can easily check -- see 
e.g. https://gcc.godbolt.org/z/ETeGzc3ve (crashes at this commit, changes to an 
error at ce861ec782ae3f41807b61e855512aaccf3c2149 
).

I'm not deep enough into this nook of the language to tell if the bug is here 
or libstdc++, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-05-01 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@erichkeane - thanks! I'll send a diff for the release notes ~soon (~this 
week).  (P.S.  just in case - I'll be out of office for ~2 weeks)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-28 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

Hey! I wish you'd given me a chance to review this fix version before 
committing!  I don't have concerns now looking at it, however please add a 
release note, since this is fixing a whole-bunch of regressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-27 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe3b1083e00e6: [Clang][Sema] Fix comparison of constraint 
expressions (authored by alexander-shaposhnikov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/AST/DeclTemplate.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-friends.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,62 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+}
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,220 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-27 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 517513.
alexander-shaposhnikov added a comment.

Add more tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/AST/DeclTemplate.h
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-friends.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,62 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+}
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,220 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-26 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

/* will update the diff ~soon */


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-26 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov planned changes to this revision.
alexander-shaposhnikov added a comment.

/* working on it */


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-21 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

reduced test case (that is currently blocking this diff) :

  namespace outer::internal {
  
  template 
  concept myconcept = true;
  
  }
  
  namespace outer {
  
  template  class Foo;
  
  template  struct Bar {
template  friend class Foo;
  };
  
  Bar S;
  
  namespace internal {
  int f(Foo);
  }
  
  template  struct Foo {
friend int internal::f(Foo);
  };
  
  } // namespace outer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-19 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

Yeah, things appear to work for the current version of this diff (including 
GH62110).
However, while doing some internal testing I've discovered one suspicious issue 
(sigh, sigh), but haven't been able to create a standalone repro yet.
Need more time for debugging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-19 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

Sorry I haven't been able to help yet, I'm STILL dealing with IT issues with 
the server I work on.  I appreciate your patience!

I have no comments, this all looks fine to me. Does this fix 62110 as well?  Is 
there anythign to hold this up from landing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-18 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+return SubstitutedAtomicExpr;

alexander-shaposhnikov wrote:
> alexander-shaposhnikov wrote:
> > erichkeane wrote:
> > > alexander-shaposhnikov wrote:
> > > > erichkeane wrote:
> > > > > alexander-shaposhnikov wrote:
> > > > > > erichkeane wrote:
> > > > > > > So this bit is concerning to me... we have been catching a ton of 
> > > > > > > bugs in the MLTAL stuff by trying to constant evaluate an 
> > > > > > > expression that isn't correctly constexpr, and this will defeat 
> > > > > > > it.  We shouldn't be calling this function at all on 
> > > > > > > non-fully-instantiated expressions.  What is the case that ends 
> > > > > > > up coming through here, and should be be calling this at all?
> > > > > > This happens e.g. for concepts-PR54629.cpp
> > > > > > 
> > > > > > ```
> > > > > > Old:
> > > > > > FunctionDecl 0x64d90378 
> > > > > > llvm-project/clang/test/SemaTemplate/concepts-PR54629.cpp:30:1, 
> > > > > > line:32:2> line:30:6 foo 'void ()'
> > > > > > |-RequiresExpr 0x64d90318  'bool'
> > > > > > | |-ParmVarDecl 0x64d90150  col:24 referenced t 
> > > > > > 'T &'
> > > > > > | `-NestedRequirement 0x64d902d8 dependent
> > > > > > |   `-BinaryOperator 0x64d902b8  'bool' '<'
> > > > > > | |-UnaryExprOrTypeTraitExpr 0x64d90260  
> > > > > > 'unsigned long' sizeof
> > > > > > | | `-ParenExpr 0x64d90240  'T' lvalue
> > > > > > | |   `-DeclRefExpr 0x64d90220  'T' lvalue ParmVar 
> > > > > > 0x64d90150 't' 'T &' non_odr_use_unevaluated
> > > > > > | `-ImplicitCastExpr 0x64d902a0  'unsigned long' 
> > > > > > 
> > > > > > |   `-IntegerLiteral 0x64d90280  'int' 4
> > > > > > `-CompoundStmt 0x64d90f70 
> > > > > > 
> > > > > > while MLTAL is empty.
> > > > > > ```
> > > > > > (clang::Sema::CheckOverload calls clang::Sema::IsOverload, while 
> > > > > > clang::Sema::IsOverload invokes 
> > > > > > clang::Sema::AreConstraintExpressionsEqual)
> > > > > Hmm that seems wrong for me, but I'm not sure how.  It doesn't 
> > > > > seem right for `AreConstraintExpressionsEqual`to try to calculate the 
> > > > > constraint satisfaction...
> > > > I think we get there from 
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateInstantiate.cpp#L2375
> > > So I still think this is an incorrect change.  I don't understand why 
> > > we'd get here without the 'final' check, but perhaps there is something 
> > > missing elsewhere?
> > unfortunately, I don't have enough context - will revisit this change (iirc 
> > one test was failing without it and Clang was calling 
> > CheckConstraintSatisfaction from an unexpected place). There is some 
> > accumulated technical debt (without the current diff) / it takes to 
> > untangle.  
> reduced test case (extracted from concepts-PR54629.cpp):
> 
> ```
> template 
> void foo()
>   requires requires(T ) { requires sizeof(t) < 4; }
> {}
> 
> template 
> void foo()
>   requires requires(T ) { requires sizeof(t) > 4; }
> {}
> 
> void test() {
>   foo();
> }
> ```
so I've investigated a bit how things work right now (without this diff),
the following code is somewhat (but not directly) relevant:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaConcept.cpp#L417
 
(from 
https://github.com/llvm/llvm-project/commit/b25902736c2e330429278e1929cc5afd2201fb77)
 
however, previously, the issues above (for the previous version of this diff) 
(e.g. for concepts-PR54629.cpp) were triggered when Clang was trying to 
substitute an empty MLTAL, while in our case it's unnecessary. 
It might happen though that there is another problem lingering around but it 
looks like at the moment the tests are passing without changes to 
```
calculateConstraintSatisfaction
```
(=> dropped them)
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-18 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:781
+  /*ForConstraintInstantiation=*/true, /*SkipForSpecialization*/ false);
+  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
+  std::optional ThisScope;

alexander-shaposhnikov wrote:
> rsmith wrote:
> > Just a small optimization: there's no point doing the transform if we have 
> > nothing to substitute.
> > Just a small optimization: there's no point doing the transform if we have 
> > nothing to substitute.
> 
> 
do not try to substitute empty MLTAL (suggested above by @rsmith)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-18 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:781
+  /*ForConstraintInstantiation=*/true, /*SkipForSpecialization*/ false);
+  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
+  std::optional ThisScope;

rsmith wrote:
> Just a small optimization: there's no point doing the transform if we have 
> nothing to substitute.
> Just a small optimization: there's no point doing the transform if we have 
> nothing to substitute.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-18 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 514745.
alexander-shaposhnikov added a comment.

New version (address some comments)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,62 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+}
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,220 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-17 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+return SubstitutedAtomicExpr;

alexander-shaposhnikov wrote:
> erichkeane wrote:
> > alexander-shaposhnikov wrote:
> > > erichkeane wrote:
> > > > alexander-shaposhnikov wrote:
> > > > > erichkeane wrote:
> > > > > > So this bit is concerning to me... we have been catching a ton of 
> > > > > > bugs in the MLTAL stuff by trying to constant evaluate an 
> > > > > > expression that isn't correctly constexpr, and this will defeat it. 
> > > > > >  We shouldn't be calling this function at all on 
> > > > > > non-fully-instantiated expressions.  What is the case that ends up 
> > > > > > coming through here, and should be be calling this at all?
> > > > > This happens e.g. for concepts-PR54629.cpp
> > > > > 
> > > > > ```
> > > > > Old:
> > > > > FunctionDecl 0x64d90378 
> > > > > llvm-project/clang/test/SemaTemplate/concepts-PR54629.cpp:30:1, 
> > > > > line:32:2> line:30:6 foo 'void ()'
> > > > > |-RequiresExpr 0x64d90318  'bool'
> > > > > | |-ParmVarDecl 0x64d90150  col:24 referenced t 
> > > > > 'T &'
> > > > > | `-NestedRequirement 0x64d902d8 dependent
> > > > > |   `-BinaryOperator 0x64d902b8  'bool' '<'
> > > > > | |-UnaryExprOrTypeTraitExpr 0x64d90260  
> > > > > 'unsigned long' sizeof
> > > > > | | `-ParenExpr 0x64d90240  'T' lvalue
> > > > > | |   `-DeclRefExpr 0x64d90220  'T' lvalue ParmVar 
> > > > > 0x64d90150 't' 'T &' non_odr_use_unevaluated
> > > > > | `-ImplicitCastExpr 0x64d902a0  'unsigned long' 
> > > > > 
> > > > > |   `-IntegerLiteral 0x64d90280  'int' 4
> > > > > `-CompoundStmt 0x64d90f70 
> > > > > 
> > > > > while MLTAL is empty.
> > > > > ```
> > > > > (clang::Sema::CheckOverload calls clang::Sema::IsOverload, while 
> > > > > clang::Sema::IsOverload invokes 
> > > > > clang::Sema::AreConstraintExpressionsEqual)
> > > > Hmm that seems wrong for me, but I'm not sure how.  It doesn't seem 
> > > > right for `AreConstraintExpressionsEqual`to try to calculate the 
> > > > constraint satisfaction...
> > > I think we get there from 
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateInstantiate.cpp#L2375
> > So I still think this is an incorrect change.  I don't understand why we'd 
> > get here without the 'final' check, but perhaps there is something missing 
> > elsewhere?
> unfortunately, I don't have enough context - will revisit this change (iirc 
> one test was failing without it and Clang was calling 
> CheckConstraintSatisfaction from an unexpected place). There is some 
> accumulated technical debt (without the current diff) / it takes to untangle. 
>  
reduced test case (extracted from concepts-PR54629.cpp):

```
template 
void foo()
  requires requires(T ) { requires sizeof(t) < 4; }
{}

template 
void foo()
  requires requires(T ) { requires sizeof(t) > 4; }
{}

void test() {
  foo();
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-17 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+return SubstitutedAtomicExpr;

erichkeane wrote:
> alexander-shaposhnikov wrote:
> > erichkeane wrote:
> > > alexander-shaposhnikov wrote:
> > > > erichkeane wrote:
> > > > > So this bit is concerning to me... we have been catching a ton of 
> > > > > bugs in the MLTAL stuff by trying to constant evaluate an expression 
> > > > > that isn't correctly constexpr, and this will defeat it.  We 
> > > > > shouldn't be calling this function at all on non-fully-instantiated 
> > > > > expressions.  What is the case that ends up coming through here, and 
> > > > > should be be calling this at all?
> > > > This happens e.g. for concepts-PR54629.cpp
> > > > 
> > > > ```
> > > > Old:
> > > > FunctionDecl 0x64d90378 
> > > > llvm-project/clang/test/SemaTemplate/concepts-PR54629.cpp:30:1, 
> > > > line:32:2> line:30:6 foo 'void ()'
> > > > |-RequiresExpr 0x64d90318  'bool'
> > > > | |-ParmVarDecl 0x64d90150  col:24 referenced t 'T 
> > > > &'
> > > > | `-NestedRequirement 0x64d902d8 dependent
> > > > |   `-BinaryOperator 0x64d902b8  'bool' '<'
> > > > | |-UnaryExprOrTypeTraitExpr 0x64d90260  
> > > > 'unsigned long' sizeof
> > > > | | `-ParenExpr 0x64d90240  'T' lvalue
> > > > | |   `-DeclRefExpr 0x64d90220  'T' lvalue ParmVar 
> > > > 0x64d90150 't' 'T &' non_odr_use_unevaluated
> > > > | `-ImplicitCastExpr 0x64d902a0  'unsigned long' 
> > > > 
> > > > |   `-IntegerLiteral 0x64d90280  'int' 4
> > > > `-CompoundStmt 0x64d90f70 
> > > > 
> > > > while MLTAL is empty.
> > > > ```
> > > > (clang::Sema::CheckOverload calls clang::Sema::IsOverload, while 
> > > > clang::Sema::IsOverload invokes 
> > > > clang::Sema::AreConstraintExpressionsEqual)
> > > Hmm that seems wrong for me, but I'm not sure how.  It doesn't seem 
> > > right for `AreConstraintExpressionsEqual`to try to calculate the 
> > > constraint satisfaction...
> > I think we get there from 
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateInstantiate.cpp#L2375
> So I still think this is an incorrect change.  I don't understand why we'd 
> get here without the 'final' check, but perhaps there is something missing 
> elsewhere?
unfortunately, I don't have enough context - will revisit this change (iirc one 
test was failing without it and Clang was calling CheckConstraintSatisfaction 
from an unexpected place). There is some accumulated technical debt (without 
the current diff) / it takes to untangle.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-17 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+return SubstitutedAtomicExpr;

alexander-shaposhnikov wrote:
> erichkeane wrote:
> > alexander-shaposhnikov wrote:
> > > erichkeane wrote:
> > > > So this bit is concerning to me... we have been catching a ton of bugs 
> > > > in the MLTAL stuff by trying to constant evaluate an expression that 
> > > > isn't correctly constexpr, and this will defeat it.  We shouldn't be 
> > > > calling this function at all on non-fully-instantiated expressions.  
> > > > What is the case that ends up coming through here, and should be be 
> > > > calling this at all?
> > > This happens e.g. for concepts-PR54629.cpp
> > > 
> > > ```
> > > Old:
> > > FunctionDecl 0x64d90378 
> > > llvm-project/clang/test/SemaTemplate/concepts-PR54629.cpp:30:1, 
> > > line:32:2> line:30:6 foo 'void ()'
> > > |-RequiresExpr 0x64d90318  'bool'
> > > | |-ParmVarDecl 0x64d90150  col:24 referenced t 'T &'
> > > | `-NestedRequirement 0x64d902d8 dependent
> > > |   `-BinaryOperator 0x64d902b8  'bool' '<'
> > > | |-UnaryExprOrTypeTraitExpr 0x64d90260  
> > > 'unsigned long' sizeof
> > > | | `-ParenExpr 0x64d90240  'T' lvalue
> > > | |   `-DeclRefExpr 0x64d90220  'T' lvalue ParmVar 
> > > 0x64d90150 't' 'T &' non_odr_use_unevaluated
> > > | `-ImplicitCastExpr 0x64d902a0  'unsigned long' 
> > > 
> > > |   `-IntegerLiteral 0x64d90280  'int' 4
> > > `-CompoundStmt 0x64d90f70 
> > > 
> > > while MLTAL is empty.
> > > ```
> > > (clang::Sema::CheckOverload calls clang::Sema::IsOverload, while 
> > > clang::Sema::IsOverload invokes 
> > > clang::Sema::AreConstraintExpressionsEqual)
> > Hmm that seems wrong for me, but I'm not sure how.  It doesn't seem 
> > right for `AreConstraintExpressionsEqual`to try to calculate the constraint 
> > satisfaction...
> I think we get there from 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateInstantiate.cpp#L2375
So I still think this is an incorrect change.  I don't understand why we'd get 
here without the 'final' check, but perhaps there is something missing 
elsewhere?



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:227
+(TSTy = Ty->getAs()))
+  Result.addOuterTemplateArguments(const_cast(FTD),
+   TSTy->template_arguments(),

alexander-shaposhnikov wrote:
> erichkeane wrote:
> > So I'd come up with something similar, but this ends up being a little 
> > goofy?  And I thought it didn't really work in 1 of the cases.  I wonder if 
> > we're better off looking at the decl contexts to try to figure out WHICH of 
> > the cases we are, and just set the next decl context based on that?
> sure, we can try. In the meantime - regarding GH62110 - do you happen to have 
> any thoughts on that ?
There is nothing that sticks out from reading it.  I'm unfortunately without my 
work machine for Friday/today, so I haven't had time to spend in the debugger.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-17 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:227
+(TSTy = Ty->getAs()))
+  Result.addOuterTemplateArguments(const_cast(FTD),
+   TSTy->template_arguments(),

erichkeane wrote:
> So I'd come up with something similar, but this ends up being a little goofy? 
>  And I thought it didn't really work in 1 of the cases.  I wonder if we're 
> better off looking at the decl contexts to try to figure out WHICH of the 
> cases we are, and just set the next decl context based on that?
sure, we can try. In the meantime - regarding GH62110 - do you happen to have 
any thoughts on that ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-17 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:227
+(TSTy = Ty->getAs()))
+  Result.addOuterTemplateArguments(const_cast(FTD),
+   TSTy->template_arguments(),

So I'd come up with something similar, but this ends up being a little goofy?  
And I thought it didn't really work in 1 of the cases.  I wonder if we're 
better off looking at the decl contexts to try to figure out WHICH of the cases 
we are, and just set the next decl context based on that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-16 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

https://github.com/llvm/llvm-project/issues/62110 is still a blocker for this 
diff though (haven't investigated it yet)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-16 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 514095.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,62 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+}
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,220 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-13 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov planned changes to this revision.
alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

erichkeane wrote:
> erichkeane wrote:
> > erichkeane wrote:
> > > alexander-shaposhnikov wrote:
> > > > erichkeane wrote:
> > > > > alexander-shaposhnikov wrote:
> > > > > > erichkeane wrote:
> > > > > > > alexander-shaposhnikov wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > Hmm... this seems really strange to have to do. 
> > > > > > > > > `ForConstraintInstantiation` shouldn't be used here, the 
> > > > > > > > > point of that is to make sure we 'keep looking upward' once 
> > > > > > > > > we hit a spot we normally stop with.  What exactly is the 
> > > > > > > > > issue that you end up running into here?  Perhaps I can spend 
> > > > > > > > > some time debugging what we should really be doign.
> > > > > > > > yeah, I agree. I haven't found a proper solution or at least a 
> > > > > > > > better workaround (but would be happy to).
> > > > > > > > This kicks in for the case 
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > template 
> > > > > > > > concept Constraint = true;
> > > > > > > > 
> > > > > > > > 
> > > > > > > > template
> > > > > > > > struct Iterator {
> > > > > > > > template 
> > > > > > > > friend class Iterator;
> > > > > > > > void operator*();
> > > > > > > > };
> > > > > > > > 
> > > > > > > > Iterator I2;
> > > > > > > > ```
> > > > > > > > yeah, I agree. I haven't found a proper solution or at least a 
> > > > > > > > better workaround (but would be happy to).
> > > > > > > > This kicks in for the case 
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > template 
> > > > > > > > concept Constraint = true;
> > > > > > > > 
> > > > > > > > 
> > > > > > > > template
> > > > > > > > struct Iterator {
> > > > > > > > template 
> > > > > > > > friend class Iterator;
> > > > > > > > void operator*();
> > > > > > > > };
> > > > > > > > 
> > > > > > > > Iterator I2;
> > > > > > > > ```
> > > > > > > 
> > > > > > > Alright, well, I should have time later in the week to poke at 
> > > > > > > this, perhaps I can come up with something better?  I DO remember 
> > > > > > > self-friend is a little wacky, and I spent a bunch of time on it 
> > > > > > > last time.
> > > > > > Ok, sounds good + maybe Richard will give it another look.
> > > > > So IMO, `ForConstraintInstantiation` should be 'true' always, and 
> > > > > that makes those examples pass.  However, I'm now seeing that it 
> > > > > causes a failure in the concepts-out-of-line-def.cpp file.  
> > > > > 
> > > > > I took the example of `foo3`:
> > > > > 
> > > > > ```
> > > > >template concept C = true;
> > > > >template 
> > > > >struct S {
> > > > >  template  requires C
> > > > >  void foo3(F3 f); // #1
> > > > >};
> > > > >template 
> > > > >template  requires C
> > > > >void S::foo3(F6 f) {} // #3
> > > > > ```
> > > > > 
> > > > > Which, seems to require `ForConstraintInstantiation` to be false to 
> > > > > pass.  However, I don't think this is correct.  This is only working 
> > > > > because when evaluating the in-line one (#1 above!) its skipping the 
> > > > > application of `T1`, which is wrong.  
> > > > > 
> > > > > However, I think the problem here is that the `out of line` version 
> > > > > (#3) is not applying the T4 like it should be. SO, I think the 
> > > > > `HandleFunctionTemplateDecl` I provided you earlier needs 
> > > > > modification.
> > > > > 
> > > > > FIRST, though not related to this, I think we might need to add 
> > > > > `FunctionTemplateDecl::getInjectedTemplateArgs` to the `Result`, but 
> > > > > only because that 'sounds right' to me?  IDK what other problem that 
> > > > > would cause, but it is worth evaluating/saving for later.  It might 
> > > > > just not matter, since we're treating them as equal at the moment, I 
> > > > > don't think injecting them would cause anything.
> > > > > 
> > > > > Secondly: I see that the we can get to the `T4` via the 
> > > > > `FTD->getTemplatedDecl()->getQualifier()->getAsType()->getAs()->template_arguments()`.
> > > > >   
> > > > > 
> > > > > HOWEVER, the problem that comes with picking THOSE up, is that it 
> > > > > ALSO applies with a `FunctionTemplateDecl` inside of an out-of-line 
> > > > > `ClassTemplateSpecializationDecl` (which doesn't NEED the 
> > > > > specialization's template args).  
> > > > > 
> > > > > SO I think the fix here is something in `HandleFunctionTemplateDecl` 
> > > > > to make sure we pick up the right list of template arguments.  I 
> > > > > haven't figured out the magic incantation to make it work 
> > > > > unfortunately, perhaps @rsmith has some hints?  Else I'll keep poking 
> > > > > at it.
> > > > > 
> > > > > BUT, my

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-13 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

erichkeane wrote:
> erichkeane wrote:
> > alexander-shaposhnikov wrote:
> > > erichkeane wrote:
> > > > alexander-shaposhnikov wrote:
> > > > > erichkeane wrote:
> > > > > > alexander-shaposhnikov wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > Hmm... this seems really strange to have to do. 
> > > > > > > > `ForConstraintInstantiation` shouldn't be used here, the point 
> > > > > > > > of that is to make sure we 'keep looking upward' once we hit a 
> > > > > > > > spot we normally stop with.  What exactly is the issue that you 
> > > > > > > > end up running into here?  Perhaps I can spend some time 
> > > > > > > > debugging what we should really be doign.
> > > > > > > yeah, I agree. I haven't found a proper solution or at least a 
> > > > > > > better workaround (but would be happy to).
> > > > > > > This kicks in for the case 
> > > > > > > 
> > > > > > > ```
> > > > > > > template 
> > > > > > > concept Constraint = true;
> > > > > > > 
> > > > > > > 
> > > > > > > template
> > > > > > > struct Iterator {
> > > > > > > template 
> > > > > > > friend class Iterator;
> > > > > > > void operator*();
> > > > > > > };
> > > > > > > 
> > > > > > > Iterator I2;
> > > > > > > ```
> > > > > > > yeah, I agree. I haven't found a proper solution or at least a 
> > > > > > > better workaround (but would be happy to).
> > > > > > > This kicks in for the case 
> > > > > > > 
> > > > > > > ```
> > > > > > > template 
> > > > > > > concept Constraint = true;
> > > > > > > 
> > > > > > > 
> > > > > > > template
> > > > > > > struct Iterator {
> > > > > > > template 
> > > > > > > friend class Iterator;
> > > > > > > void operator*();
> > > > > > > };
> > > > > > > 
> > > > > > > Iterator I2;
> > > > > > > ```
> > > > > > 
> > > > > > Alright, well, I should have time later in the week to poke at 
> > > > > > this, perhaps I can come up with something better?  I DO remember 
> > > > > > self-friend is a little wacky, and I spent a bunch of time on it 
> > > > > > last time.
> > > > > Ok, sounds good + maybe Richard will give it another look.
> > > > So IMO, `ForConstraintInstantiation` should be 'true' always, and that 
> > > > makes those examples pass.  However, I'm now seeing that it causes a 
> > > > failure in the concepts-out-of-line-def.cpp file.  
> > > > 
> > > > I took the example of `foo3`:
> > > > 
> > > > ```
> > > >template concept C = true;
> > > >template 
> > > >struct S {
> > > >  template  requires C
> > > >  void foo3(F3 f); // #1
> > > >};
> > > >template 
> > > >template  requires C
> > > >void S::foo3(F6 f) {} // #3
> > > > ```
> > > > 
> > > > Which, seems to require `ForConstraintInstantiation` to be false to 
> > > > pass.  However, I don't think this is correct.  This is only working 
> > > > because when evaluating the in-line one (#1 above!) its skipping the 
> > > > application of `T1`, which is wrong.  
> > > > 
> > > > However, I think the problem here is that the `out of line` version 
> > > > (#3) is not applying the T4 like it should be. SO, I think the 
> > > > `HandleFunctionTemplateDecl` I provided you earlier needs modification.
> > > > 
> > > > FIRST, though not related to this, I think we might need to add 
> > > > `FunctionTemplateDecl::getInjectedTemplateArgs` to the `Result`, but 
> > > > only because that 'sounds right' to me?  IDK what other problem that 
> > > > would cause, but it is worth evaluating/saving for later.  It might 
> > > > just not matter, since we're treating them as equal at the moment, I 
> > > > don't think injecting them would cause anything.
> > > > 
> > > > Secondly: I see that the we can get to the `T4` via the 
> > > > `FTD->getTemplatedDecl()->getQualifier()->getAsType()->getAs()->template_arguments()`.
> > > >   
> > > > 
> > > > HOWEVER, the problem that comes with picking THOSE up, is that it ALSO 
> > > > applies with a `FunctionTemplateDecl` inside of an out-of-line 
> > > > `ClassTemplateSpecializationDecl` (which doesn't NEED the 
> > > > specialization's template args).  
> > > > 
> > > > SO I think the fix here is something in `HandleFunctionTemplateDecl` to 
> > > > make sure we pick up the right list of template arguments.  I haven't 
> > > > figured out the magic incantation to make it work unfortunately, 
> > > > perhaps @rsmith has some hints?  Else I'll keep poking at it.
> > > > 
> > > > BUT, my debugging has shown me that I'm quite convinced the setting 
> > > > `ForConstraintInstantiation ` to false is incorrect.
> > > p.s. Richard is on vacation.
> > > 
> > > to quote his comment above (where the story began):
> > > 
> > > >The right way to fix that and the issue being addressed here is that, 
> > >

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-13 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

erichkeane wrote:
> alexander-shaposhnikov wrote:
> > erichkeane wrote:
> > > alexander-shaposhnikov wrote:
> > > > erichkeane wrote:
> > > > > alexander-shaposhnikov wrote:
> > > > > > erichkeane wrote:
> > > > > > > Hmm... this seems really strange to have to do. 
> > > > > > > `ForConstraintInstantiation` shouldn't be used here, the point of 
> > > > > > > that is to make sure we 'keep looking upward' once we hit a spot 
> > > > > > > we normally stop with.  What exactly is the issue that you end up 
> > > > > > > running into here?  Perhaps I can spend some time debugging what 
> > > > > > > we should really be doign.
> > > > > > yeah, I agree. I haven't found a proper solution or at least a 
> > > > > > better workaround (but would be happy to).
> > > > > > This kicks in for the case 
> > > > > > 
> > > > > > ```
> > > > > > template 
> > > > > > concept Constraint = true;
> > > > > > 
> > > > > > 
> > > > > > template
> > > > > > struct Iterator {
> > > > > > template 
> > > > > > friend class Iterator;
> > > > > > void operator*();
> > > > > > };
> > > > > > 
> > > > > > Iterator I2;
> > > > > > ```
> > > > > > yeah, I agree. I haven't found a proper solution or at least a 
> > > > > > better workaround (but would be happy to).
> > > > > > This kicks in for the case 
> > > > > > 
> > > > > > ```
> > > > > > template 
> > > > > > concept Constraint = true;
> > > > > > 
> > > > > > 
> > > > > > template
> > > > > > struct Iterator {
> > > > > > template 
> > > > > > friend class Iterator;
> > > > > > void operator*();
> > > > > > };
> > > > > > 
> > > > > > Iterator I2;
> > > > > > ```
> > > > > 
> > > > > Alright, well, I should have time later in the week to poke at this, 
> > > > > perhaps I can come up with something better?  I DO remember 
> > > > > self-friend is a little wacky, and I spent a bunch of time on it last 
> > > > > time.
> > > > Ok, sounds good + maybe Richard will give it another look.
> > > So IMO, `ForConstraintInstantiation` should be 'true' always, and that 
> > > makes those examples pass.  However, I'm now seeing that it causes a 
> > > failure in the concepts-out-of-line-def.cpp file.  
> > > 
> > > I took the example of `foo3`:
> > > 
> > > ```
> > >template concept C = true;
> > >template 
> > >struct S {
> > >  template  requires C
> > >  void foo3(F3 f); // #1
> > >};
> > >template 
> > >template  requires C
> > >void S::foo3(F6 f) {} // #3
> > > ```
> > > 
> > > Which, seems to require `ForConstraintInstantiation` to be false to pass. 
> > >  However, I don't think this is correct.  This is only working because 
> > > when evaluating the in-line one (#1 above!) its skipping the application 
> > > of `T1`, which is wrong.  
> > > 
> > > However, I think the problem here is that the `out of line` version (#3) 
> > > is not applying the T4 like it should be. SO, I think the 
> > > `HandleFunctionTemplateDecl` I provided you earlier needs modification.
> > > 
> > > FIRST, though not related to this, I think we might need to add 
> > > `FunctionTemplateDecl::getInjectedTemplateArgs` to the `Result`, but only 
> > > because that 'sounds right' to me?  IDK what other problem that would 
> > > cause, but it is worth evaluating/saving for later.  It might just not 
> > > matter, since we're treating them as equal at the moment, I don't think 
> > > injecting them would cause anything.
> > > 
> > > Secondly: I see that the we can get to the `T4` via the 
> > > `FTD->getTemplatedDecl()->getQualifier()->getAsType()->getAs()->template_arguments()`.
> > >   
> > > 
> > > HOWEVER, the problem that comes with picking THOSE up, is that it ALSO 
> > > applies with a `FunctionTemplateDecl` inside of an out-of-line 
> > > `ClassTemplateSpecializationDecl` (which doesn't NEED the 
> > > specialization's template args).  
> > > 
> > > SO I think the fix here is something in `HandleFunctionTemplateDecl` to 
> > > make sure we pick up the right list of template arguments.  I haven't 
> > > figured out the magic incantation to make it work unfortunately, perhaps 
> > > @rsmith has some hints?  Else I'll keep poking at it.
> > > 
> > > BUT, my debugging has shown me that I'm quite convinced the setting 
> > > `ForConstraintInstantiation ` to false is incorrect.
> > p.s. Richard is on vacation.
> > 
> > to quote his comment above (where the story began):
> > 
> > >The right way to fix that and the issue being addressed here is that, 
> > >rather than adjusting the depths, we ?>should substitute the outer 
> > >template arguments from the scope specifier (A::) into the constraint 
> > >before >performing the comparison. (In the special case where none of the 
> > >outer template

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-13 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

erichkeane wrote:
> alexander-shaposhnikov wrote:
> > erichkeane wrote:
> > > alexander-shaposhnikov wrote:
> > > > erichkeane wrote:
> > > > > Hmm... this seems really strange to have to do. 
> > > > > `ForConstraintInstantiation` shouldn't be used here, the point of 
> > > > > that is to make sure we 'keep looking upward' once we hit a spot we 
> > > > > normally stop with.  What exactly is the issue that you end up 
> > > > > running into here?  Perhaps I can spend some time debugging what we 
> > > > > should really be doign.
> > > > yeah, I agree. I haven't found a proper solution or at least a better 
> > > > workaround (but would be happy to).
> > > > This kicks in for the case 
> > > > 
> > > > ```
> > > > template 
> > > > concept Constraint = true;
> > > > 
> > > > 
> > > > template
> > > > struct Iterator {
> > > > template 
> > > > friend class Iterator;
> > > > void operator*();
> > > > };
> > > > 
> > > > Iterator I2;
> > > > ```
> > > > yeah, I agree. I haven't found a proper solution or at least a better 
> > > > workaround (but would be happy to).
> > > > This kicks in for the case 
> > > > 
> > > > ```
> > > > template 
> > > > concept Constraint = true;
> > > > 
> > > > 
> > > > template
> > > > struct Iterator {
> > > > template 
> > > > friend class Iterator;
> > > > void operator*();
> > > > };
> > > > 
> > > > Iterator I2;
> > > > ```
> > > 
> > > Alright, well, I should have time later in the week to poke at this, 
> > > perhaps I can come up with something better?  I DO remember self-friend 
> > > is a little wacky, and I spent a bunch of time on it last time.
> > Ok, sounds good + maybe Richard will give it another look.
> So IMO, `ForConstraintInstantiation` should be 'true' always, and that makes 
> those examples pass.  However, I'm now seeing that it causes a failure in the 
> concepts-out-of-line-def.cpp file.  
> 
> I took the example of `foo3`:
> 
> ```
>template concept C = true;
>template 
>struct S {
>  template  requires C
>  void foo3(F3 f); // #1
>};
>template 
>template  requires C
>void S::foo3(F6 f) {} // #3
> ```
> 
> Which, seems to require `ForConstraintInstantiation` to be false to pass.  
> However, I don't think this is correct.  This is only working because when 
> evaluating the in-line one (#1 above!) its skipping the application of `T1`, 
> which is wrong.  
> 
> However, I think the problem here is that the `out of line` version (#3) is 
> not applying the T4 like it should be. SO, I think the 
> `HandleFunctionTemplateDecl` I provided you earlier needs modification.
> 
> FIRST, though not related to this, I think we might need to add 
> `FunctionTemplateDecl::getInjectedTemplateArgs` to the `Result`, but only 
> because that 'sounds right' to me?  IDK what other problem that would cause, 
> but it is worth evaluating/saving for later.  It might just not matter, since 
> we're treating them as equal at the moment, I don't think injecting them 
> would cause anything.
> 
> Secondly: I see that the we can get to the `T4` via the 
> `FTD->getTemplatedDecl()->getQualifier()->getAsType()->getAs()->template_arguments()`.
>   
> 
> HOWEVER, the problem that comes with picking THOSE up, is that it ALSO 
> applies with a `FunctionTemplateDecl` inside of an out-of-line 
> `ClassTemplateSpecializationDecl` (which doesn't NEED the specialization's 
> template args).  
> 
> SO I think the fix here is something in `HandleFunctionTemplateDecl` to make 
> sure we pick up the right list of template arguments.  I haven't figured out 
> the magic incantation to make it work unfortunately, perhaps @rsmith has some 
> hints?  Else I'll keep poking at it.
> 
> BUT, my debugging has shown me that I'm quite convinced the setting 
> `ForConstraintInstantiation ` to false is incorrect.
p.s. Richard is on vacation.

to quote his comment above (where the story began):

>The right way to fix that and the issue being addressed here is that, rather 
>than adjusting the depths, we ?>should substitute the outer template arguments 
>from the scope specifier (A::) into the constraint before >performing the 
>comparison. (In the special case where none of the outer template parameters 
>are used by the >inner template, that does effectively just adjust the depths 
>of any inner template parameters.)

A. the formal meaning of ForConstraintInstantiation=true/false is unclear, 
@eirchkean - if you happen to understand it - mind expanding the comment a bit 
? (and we can add this to the documentation)

B.  T1 - maybe it should be collected as a retained layer ? 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-13 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

erichkeane wrote:
> alexander-shaposhnikov wrote:
> > erichkeane wrote:
> > > alexander-shaposhnikov wrote:
> > > > erichkeane wrote:
> > > > > Hmm... this seems really strange to have to do. 
> > > > > `ForConstraintInstantiation` shouldn't be used here, the point of 
> > > > > that is to make sure we 'keep looking upward' once we hit a spot we 
> > > > > normally stop with.  What exactly is the issue that you end up 
> > > > > running into here?  Perhaps I can spend some time debugging what we 
> > > > > should really be doign.
> > > > yeah, I agree. I haven't found a proper solution or at least a better 
> > > > workaround (but would be happy to).
> > > > This kicks in for the case 
> > > > 
> > > > ```
> > > > template 
> > > > concept Constraint = true;
> > > > 
> > > > 
> > > > template
> > > > struct Iterator {
> > > > template 
> > > > friend class Iterator;
> > > > void operator*();
> > > > };
> > > > 
> > > > Iterator I2;
> > > > ```
> > > > yeah, I agree. I haven't found a proper solution or at least a better 
> > > > workaround (but would be happy to).
> > > > This kicks in for the case 
> > > > 
> > > > ```
> > > > template 
> > > > concept Constraint = true;
> > > > 
> > > > 
> > > > template
> > > > struct Iterator {
> > > > template 
> > > > friend class Iterator;
> > > > void operator*();
> > > > };
> > > > 
> > > > Iterator I2;
> > > > ```
> > > 
> > > Alright, well, I should have time later in the week to poke at this, 
> > > perhaps I can come up with something better?  I DO remember self-friend 
> > > is a little wacky, and I spent a bunch of time on it last time.
> > Ok, sounds good + maybe Richard will give it another look.
> So IMO, `ForConstraintInstantiation` should be 'true' always, and that makes 
> those examples pass.  However, I'm now seeing that it causes a failure in the 
> concepts-out-of-line-def.cpp file.  
> 
> I took the example of `foo3`:
> 
> ```
>template concept C = true;
>template 
>struct S {
>  template  requires C
>  void foo3(F3 f); // #1
>};
>template 
>template  requires C
>void S::foo3(F6 f) {} // #3
> ```
> 
> Which, seems to require `ForConstraintInstantiation` to be false to pass.  
> However, I don't think this is correct.  This is only working because when 
> evaluating the in-line one (#1 above!) its skipping the application of `T1`, 
> which is wrong.  
> 
> However, I think the problem here is that the `out of line` version (#3) is 
> not applying the T4 like it should be. SO, I think the 
> `HandleFunctionTemplateDecl` I provided you earlier needs modification.
> 
> FIRST, though not related to this, I think we might need to add 
> `FunctionTemplateDecl::getInjectedTemplateArgs` to the `Result`, but only 
> because that 'sounds right' to me?  IDK what other problem that would cause, 
> but it is worth evaluating/saving for later.  It might just not matter, since 
> we're treating them as equal at the moment, I don't think injecting them 
> would cause anything.
> 
> Secondly: I see that the we can get to the `T4` via the 
> `FTD->getTemplatedDecl()->getQualifier()->getAsType()->getAs()->template_arguments()`.
>   
> 
> HOWEVER, the problem that comes with picking THOSE up, is that it ALSO 
> applies with a `FunctionTemplateDecl` inside of an out-of-line 
> `ClassTemplateSpecializationDecl` (which doesn't NEED the specialization's 
> template args).  
> 
> SO I think the fix here is something in `HandleFunctionTemplateDecl` to make 
> sure we pick up the right list of template arguments.  I haven't figured out 
> the magic incantation to make it work unfortunately, perhaps @rsmith has some 
> hints?  Else I'll keep poking at it.
> 
> BUT, my debugging has shown me that I'm quite convinced the setting 
> `ForConstraintInstantiation ` to false is incorrect.
I also notice that the work in `HandleRecordDecl` inside of the 
`ForConstraintInstantiation` is a little odd, I don't recall why I put that in 
place, but it DOES result in 5 failed tests if I remove it.  Perhaps we just 
need to be more careful when we substitute in these arguments?  It seems to me 
that putting those arguments in place is perhaps not the right thing to do, 
since they don't have any meaningful values yet?  Perhaps something else for us 
to poke at.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-13 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.

Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

alexander-shaposhnikov wrote:
> erichkeane wrote:
> > alexander-shaposhnikov wrote:
> > > erichkeane wrote:
> > > > Hmm... this seems really strange to have to do. 
> > > > `ForConstraintInstantiation` shouldn't be used here, the point of that 
> > > > is to make sure we 'keep looking upward' once we hit a spot we normally 
> > > > stop with.  What exactly is the issue that you end up running into 
> > > > here?  Perhaps I can spend some time debugging what we should really be 
> > > > doign.
> > > yeah, I agree. I haven't found a proper solution or at least a better 
> > > workaround (but would be happy to).
> > > This kicks in for the case 
> > > 
> > > ```
> > > template 
> > > concept Constraint = true;
> > > 
> > > 
> > > template
> > > struct Iterator {
> > > template 
> > > friend class Iterator;
> > > void operator*();
> > > };
> > > 
> > > Iterator I2;
> > > ```
> > > yeah, I agree. I haven't found a proper solution or at least a better 
> > > workaround (but would be happy to).
> > > This kicks in for the case 
> > > 
> > > ```
> > > template 
> > > concept Constraint = true;
> > > 
> > > 
> > > template
> > > struct Iterator {
> > > template 
> > > friend class Iterator;
> > > void operator*();
> > > };
> > > 
> > > Iterator I2;
> > > ```
> > 
> > Alright, well, I should have time later in the week to poke at this, 
> > perhaps I can come up with something better?  I DO remember self-friend is 
> > a little wacky, and I spent a bunch of time on it last time.
> Ok, sounds good + maybe Richard will give it another look.
So IMO, `ForConstraintInstantiation` should be 'true' always, and that makes 
those examples pass.  However, I'm now seeing that it causes a failure in the 
concepts-out-of-line-def.cpp file.  

I took the example of `foo3`:

```
   template concept C = true;
   template 
   struct S {
 template  requires C
 void foo3(F3 f); // #1
   };
   template 
   template  requires C
   void S::foo3(F6 f) {} // #3
```

Which, seems to require `ForConstraintInstantiation` to be false to pass.  
However, I don't think this is correct.  This is only working because when 
evaluating the in-line one (#1 above!) its skipping the application of `T1`, 
which is wrong.  

However, I think the problem here is that the `out of line` version (#3) is not 
applying the T4 like it should be. SO, I think the `HandleFunctionTemplateDecl` 
I provided you earlier needs modification.

FIRST, though not related to this, I think we might need to add 
`FunctionTemplateDecl::getInjectedTemplateArgs` to the `Result`, but only 
because that 'sounds right' to me?  IDK what other problem that would cause, 
but it is worth evaluating/saving for later.  It might just not matter, since 
we're treating them as equal at the moment, I don't think injecting them would 
cause anything.

Secondly: I see that the we can get to the `T4` via the 
`FTD->getTemplatedDecl()->getQualifier()->getAsType()->getAs()->template_arguments()`.

HOWEVER, the problem that comes with picking THOSE up, is that it ALSO applies 
with a `FunctionTemplateDecl` inside of an out-of-line 
`ClassTemplateSpecializationDecl` (which doesn't NEED the specialization's 
template args).  

SO I think the fix here is something in `HandleFunctionTemplateDecl` to make 
sure we pick up the right list of template arguments.  I haven't figured out 
the magic incantation to make it work unfortunately, perhaps @rsmith has some 
hints?  Else I'll keep poking at it.

BUT, my debugging has shown me that I'm quite convinced the setting 
`ForConstraintInstantiation ` to false is incorrect.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-12 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

https://github.com/llvm/llvm-project/issues/62110 (in addition to the 
self-friendship case)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-12 Thread Erich Keane via Phabricator via cfe-commits

erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.

Requesting changes so we don't accidentally commit this.  There is some 
confusion elsewhere on how ready this is for commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-10 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+return SubstitutedAtomicExpr;

erichkeane wrote:
> alexander-shaposhnikov wrote:
> > erichkeane wrote:
> > > So this bit is concerning to me... we have been catching a ton of bugs in 
> > > the MLTAL stuff by trying to constant evaluate an expression that isn't 
> > > correctly constexpr, and this will defeat it.  We shouldn't be calling 
> > > this function at all on non-fully-instantiated expressions.  What is the 
> > > case that ends up coming through here, and should be be calling this at 
> > > all?
> > This happens e.g. for concepts-PR54629.cpp
> > 
> > ```
> > Old:
> > FunctionDecl 0x64d90378 
> > llvm-project/clang/test/SemaTemplate/concepts-PR54629.cpp:30:1, line:32:2> 
> > line:30:6 foo 'void ()'
> > |-RequiresExpr 0x64d90318  'bool'
> > | |-ParmVarDecl 0x64d90150  col:24 referenced t 'T &'
> > | `-NestedRequirement 0x64d902d8 dependent
> > |   `-BinaryOperator 0x64d902b8  'bool' '<'
> > | |-UnaryExprOrTypeTraitExpr 0x64d90260  'unsigned 
> > long' sizeof
> > | | `-ParenExpr 0x64d90240  'T' lvalue
> > | |   `-DeclRefExpr 0x64d90220  'T' lvalue ParmVar 
> > 0x64d90150 't' 'T &' non_odr_use_unevaluated
> > | `-ImplicitCastExpr 0x64d902a0  'unsigned long' 
> > 
> > |   `-IntegerLiteral 0x64d90280  'int' 4
> > `-CompoundStmt 0x64d90f70 
> > 
> > while MLTAL is empty.
> > ```
> > (clang::Sema::CheckOverload calls clang::Sema::IsOverload, while 
> > clang::Sema::IsOverload invokes clang::Sema::AreConstraintExpressionsEqual)
> Hmm that seems wrong for me, but I'm not sure how.  It doesn't seem right 
> for `AreConstraintExpressionsEqual`to try to calculate the constraint 
> satisfaction...
I think we get there from 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplateInstantiate.cpp#L2375


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-10 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

erichkeane wrote:
> alexander-shaposhnikov wrote:
> > erichkeane wrote:
> > > Hmm... this seems really strange to have to do. 
> > > `ForConstraintInstantiation` shouldn't be used here, the point of that is 
> > > to make sure we 'keep looking upward' once we hit a spot we normally stop 
> > > with.  What exactly is the issue that you end up running into here?  
> > > Perhaps I can spend some time debugging what we should really be doign.
> > yeah, I agree. I haven't found a proper solution or at least a better 
> > workaround (but would be happy to).
> > This kicks in for the case 
> > 
> > ```
> > template 
> > concept Constraint = true;
> > 
> > 
> > template
> > struct Iterator {
> > template 
> > friend class Iterator;
> > void operator*();
> > };
> > 
> > Iterator I2;
> > ```
> > yeah, I agree. I haven't found a proper solution or at least a better 
> > workaround (but would be happy to).
> > This kicks in for the case 
> > 
> > ```
> > template 
> > concept Constraint = true;
> > 
> > 
> > template
> > struct Iterator {
> > template 
> > friend class Iterator;
> > void operator*();
> > };
> > 
> > Iterator I2;
> > ```
> 
> Alright, well, I should have time later in the week to poke at this, perhaps 
> I can come up with something better?  I DO remember self-friend is a little 
> wacky, and I spent a bunch of time on it last time.
Ok, sounds good + maybe Richard will give it another look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-10 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

alexander-shaposhnikov wrote:
> erichkeane wrote:
> > Hmm... this seems really strange to have to do. 
> > `ForConstraintInstantiation` shouldn't be used here, the point of that is 
> > to make sure we 'keep looking upward' once we hit a spot we normally stop 
> > with.  What exactly is the issue that you end up running into here?  
> > Perhaps I can spend some time debugging what we should really be doign.
> yeah, I agree. I haven't found a proper solution or at least a better 
> workaround (but would be happy to).
> This kicks in for the case 
> 
> ```
> template 
> concept Constraint = true;
> 
> 
> template
> struct Iterator {
> template 
> friend class Iterator;
> void operator*();
> };
> 
> Iterator I2;
> ```
> yeah, I agree. I haven't found a proper solution or at least a better 
> workaround (but would be happy to).
> This kicks in for the case 
> 
> ```
> template 
> concept Constraint = true;
> 
> 
> template
> struct Iterator {
> template 
> friend class Iterator;
> void operator*();
> };
> 
> Iterator I2;
> ```

Alright, well, I should have time later in the week to poke at this, perhaps I 
can come up with something better?  I DO remember self-friend is a little 
wacky, and I spent a bunch of time on it last time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-10 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+return SubstitutedAtomicExpr;

alexander-shaposhnikov wrote:
> erichkeane wrote:
> > So this bit is concerning to me... we have been catching a ton of bugs in 
> > the MLTAL stuff by trying to constant evaluate an expression that isn't 
> > correctly constexpr, and this will defeat it.  We shouldn't be calling this 
> > function at all on non-fully-instantiated expressions.  What is the case 
> > that ends up coming through here, and should be be calling this at all?
> This happens e.g. for concepts-PR54629.cpp
> 
> ```
> Old:
> FunctionDecl 0x64d90378 
> llvm-project/clang/test/SemaTemplate/concepts-PR54629.cpp:30:1, line:32:2> 
> line:30:6 foo 'void ()'
> |-RequiresExpr 0x64d90318  'bool'
> | |-ParmVarDecl 0x64d90150  col:24 referenced t 'T &'
> | `-NestedRequirement 0x64d902d8 dependent
> |   `-BinaryOperator 0x64d902b8  'bool' '<'
> | |-UnaryExprOrTypeTraitExpr 0x64d90260  'unsigned 
> long' sizeof
> | | `-ParenExpr 0x64d90240  'T' lvalue
> | |   `-DeclRefExpr 0x64d90220  'T' lvalue ParmVar 
> 0x64d90150 't' 'T &' non_odr_use_unevaluated
> | `-ImplicitCastExpr 0x64d902a0  'unsigned long' 
> 
> |   `-IntegerLiteral 0x64d90280  'int' 4
> `-CompoundStmt 0x64d90f70 
> 
> while MLTAL is empty.
> ```
> (clang::Sema::CheckOverload calls clang::Sema::IsOverload, while 
> clang::Sema::IsOverload invokes clang::Sema::AreConstraintExpressionsEqual)
Hmm that seems wrong for me, but I'm not sure how.  It doesn't seem right 
for `AreConstraintExpressionsEqual`to try to calculate the constraint 
satisfaction...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-10 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

erichkeane wrote:
> Hmm... this seems really strange to have to do. `ForConstraintInstantiation` 
> shouldn't be used here, the point of that is to make sure we 'keep looking 
> upward' once we hit a spot we normally stop with.  What exactly is the issue 
> that you end up running into here?  Perhaps I can spend some time debugging 
> what we should really be doign.
yeah, I agree. I haven't found a proper solution or at least a better 
workaround (but would be happy to).
This kicks in for the case 

```
template 
concept Constraint = true;


template
struct Iterator {
template 
friend class Iterator;
void operator*();
};

Iterator I2;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-10 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+return SubstitutedAtomicExpr;

erichkeane wrote:
> So this bit is concerning to me... we have been catching a ton of bugs in the 
> MLTAL stuff by trying to constant evaluate an expression that isn't correctly 
> constexpr, and this will defeat it.  We shouldn't be calling this function at 
> all on non-fully-instantiated expressions.  What is the case that ends up 
> coming through here, and should be be calling this at all?
This happens e.g. for concepts-PR54629.cpp

```
Old:
FunctionDecl 0x64d90378 
llvm-project/clang/test/SemaTemplate/concepts-PR54629.cpp:30:1, line:32:2> 
line:30:6 foo 'void ()'
|-RequiresExpr 0x64d90318  'bool'
| |-ParmVarDecl 0x64d90150  col:24 referenced t 'T &'
| `-NestedRequirement 0x64d902d8 dependent
|   `-BinaryOperator 0x64d902b8  'bool' '<'
| |-UnaryExprOrTypeTraitExpr 0x64d90260  'unsigned 
long' sizeof
| | `-ParenExpr 0x64d90240  'T' lvalue
| |   `-DeclRefExpr 0x64d90220  'T' lvalue ParmVar 
0x64d90150 't' 'T &' non_odr_use_unevaluated
| `-ImplicitCastExpr 0x64d902a0  'unsigned long' 
|   `-IntegerLiteral 0x64d90280  'int' 4
`-CompoundStmt 0x64d90f70 

while MLTAL is empty.
```
(clang::Sema::CheckOverload calls clang::Sema::IsOverload, while 
clang::Sema::IsOverload invokes clang::Sema::AreConstraintExpressionsEqual)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-10 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:263
 
+  if (SubstitutedAtomicExpr.get()->isValueDependent())
+return SubstitutedAtomicExpr;

So this bit is concerning to me... we have been catching a ton of bugs in the 
MLTAL stuff by trying to constant evaluate an expression that isn't correctly 
constexpr, and this will defeat it.  We shouldn't be calling this function at 
all on non-fully-instantiated expressions.  What is the case that ends up 
coming through here, and should be be calling this at all?



Comment at: clang/lib/Sema/SemaConcept.cpp:773
+  // ConstrExpr for the inner template will properly adjust the depths.
+  if (isa(ND) && isa(OtherND))
+ForConstraintInstantiation = true;

Hmm... this seems really strange to have to do. `ForConstraintInstantiation` 
shouldn't be used here, the point of that is to make sure we 'keep looking 
upward' once we hit a spot we normally stop with.  What exactly is the issue 
that you end up running into here?  Perhaps I can spend some time debugging 
what we should really be doign.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:220
+Response HandleFunctionTemplateDecl(const FunctionTemplateDecl *FTD) {
+  return Response::ChangeDecl(FTD->getLexicalDeclContext());
+}

Huh, glad this ends up being useful!  I think i suggested this at one point in 
the last version of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-08 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 511854.
alexander-shaposhnikov added a comment.

This version is partially based on https://reviews.llvm.org/D147722.
The case with self-friendship is problematic (had to add a workaround, any 
suggestions would be greatly appreciated).
Added more tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/include/clang/Sema/Template.h
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp

Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,62 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+} // namespace TemplateInsideNonTemplateClass
+
+namespace GH61959 {
+template 
+concept C = (sizeof(T0) >= 4);
+
+template
+struct Orig { };
+
+template
+struct Orig {
+  template requires C
+  void f() { }
+
+  template requires true
+  void f() { }
+};
+
+template  struct Mod {};
+
+template 
+struct Mod {
+  template  requires C
+  constexpr static int f() { return 1; }
+
+  template  requires C
+  constexpr static int f() { return 2; }
+};
+
+static_assert(Mod::f() == 1);
+static_assert(Mod::f() == 2);
+
+template
+struct Outer {
+  template
+  struct Inner {};
+
+  template
+  struct Inner {
+template
+void foo()  requires C && C && C{}
+template
+void foo()  requires true{}
+  };
+};
+
+void bar() {
+  Outer::Inner I;
+  I.foo();
+}
+}
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,220 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-07 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

I've debugged a bit what's going on in https://godbolt.org/z/7h3sPe85h
we pass ForConstaintInstantiation=true and this causes 
us for the in-class FunctionTemplateDecl pick up the outer layer of template 
args (i.e. MLTAL will contain NumRetainedOuterLevels: 0 0: ), while for the 
out-of-line FunctionTemplateDecl MLTAL is empty. This breaks the comparison 
(after the substitution) since Clang thinks that it's removing one layer of 
template args. For example, if we pass ForConstaintInstantiation=true the code 
will compile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-06 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

This breaks the following innocent looking out-of-line definition with two 
levels of constrained template parameters:

  template  concept Result = true;
  
  template 
  class CoFuture final {
template  explicit CoFuture();
  };
  
  template 
  template 
  CoFuture::CoFuture() {}

See godbolt .

We found this during our compiler releases, not sure if there is a quick 
forward fix or we will need to rollback.
@alexander-shaposhnikov could you take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-03 Thread Alexander Shaposhnikov 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 rG60bee9ff5445: [Clang][Sema] Fix comparison of constraint 
expressions (authored by alexander-shaposhnikov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -947,9 +947,6 @@
   
 https://wg21.link/p1980r0;>P1980R0
   
-   
-https://wg21.link/p2103r0;>P2103R0
-  

 https://wg21.link/p2493r0;>P2493R0
   
@@ -961,6 +958,9 @@
 https://wg21.link/p2113r0;>P2113R0
 Clang 16
   
+  
+https://wg21.link/p2103r0;>P2103R0
+  
 
 
   Range-based for statements with initializer
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,12 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+} // namespace TemplateInsideNonTemplateClass
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,153 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-03 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 510662.
alexander-shaposhnikov added a comment.

Add test & minor optimization


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

https://reviews.llvm.org/D146178

Files:
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/test/SemaTemplate/concepts.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -947,9 +947,6 @@
   
 https://wg21.link/p1980r0;>P1980R0
   
-   
-https://wg21.link/p2103r0;>P2103R0
-  

 https://wg21.link/p2493r0;>P2493R0
   
@@ -961,6 +958,9 @@
 https://wg21.link/p2113r0;>P2113R0
 Clang 16
   
+  
+https://wg21.link/p2103r0;>P2103R0
+  
 
 
   Range-based for statements with initializer
Index: clang/test/SemaTemplate/concepts.cpp
===
--- clang/test/SemaTemplate/concepts.cpp
+++ clang/test/SemaTemplate/concepts.cpp
@@ -816,3 +816,12 @@
 static_assert(Parent::TakesBinary::i == 0);
 }
 
+namespace TemplateInsideNonTemplateClass {
+template concept C = true;
+
+template auto L = [] U>() {};
+
+struct Q {
+  template U> friend constexpr auto decltype(L)::operator()() const;
+};
+} // namespace TemplateInsideNonTemplateClass
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,153 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.
+static_assert(S<4>::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_member_sfinae
+
+namespace requires_expression_references_members {
+
+void

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-03 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 510637.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -947,9 +947,6 @@
   
 https://wg21.link/p1980r0;>P1980R0
   
-   
-https://wg21.link/p2103r0;>P2103R0
-  

 https://wg21.link/p2493r0;>P2493R0
   
@@ -961,6 +958,9 @@
 https://wg21.link/p2113r0;>P2113R0
 Clang 16
   
+  
+https://wg21.link/p2103r0;>P2103R0
+  
 
 
   Range-based for statements with initializer
Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,153 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.
+static_assert(S<4>::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_member_sfinae
+
+namespace requires_expression_references_members {
+
+void accept1(int x);
+void accept2(XY xy);
+
+template  struct S {
+  T Field = T();
+
+  constexpr int constrained_method()
+  requires requires { accept1(Field); };
+
+  constexpr int constrained_method()
+  requires requires { accept2(Field); };
+};
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept1(Field); } {
+  return CONSTRAINED_METHOD_1;
+}
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept2(Field); } {
+  return CONSTRAINED_METHOD_2;
+}
+
+static_assert(S().constrained_method() ==

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-03 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

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

Does my horrible lambda example work now? If so, that seems like a useful 
testcase.




Comment at: clang/lib/Sema/SemaConcept.cpp:781
+  /*ForConstraintInstantiation=*/true, /*SkipForSpecialization*/ false);
+  Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/false);
+  std::optional ThisScope;

Just a small optimization: there's no point doing the transform if we have 
nothing to substitute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-03 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 510634.
alexander-shaposhnikov added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp

Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,153 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.
+static_assert(S<4>::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_member_sfinae
+
+namespace requires_expression_references_members {
+
+void accept1(int x);
+void accept2(XY xy);
+
+template  struct S {
+  T Field = T();
+
+  constexpr int constrained_method()
+  requires requires { accept1(Field); };
+
+  constexpr int constrained_method()
+  requires requires { accept2(Field); };
+};
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept1(Field); } {
+  return CONSTRAINED_METHOD_1;
+}
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept2(Field); } {
+  return CONSTRAINED_METHOD_2;
+}
+
+static_assert(S().constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S().constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace requires_expression_references_members
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1653,33 +1653,12 @@
 << QualifierLoc.getSourceRange();
   return nullptr;
 }
-
-if (PrevClassTemplate) {
-  const ClassTemplateDecl *MostRecentPrevCT =
-

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-03 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:775-776
+static bool IsInsideImplicitFullTemplateSpecialization(const DeclContext *DC) {
+  auto *CTSDecl = dyn_cast_or_null(
+  DC->getOuterLexicalRecordContext());
+  return CTSDecl && !isa(CTSDecl) &&

rsmith wrote:
> This doesn't look right to me; there could be a class template nested inside 
> a non-templated class, so I think you would need to walk up the enclosing 
> `DeclContext`s one by one checking each in turn. But, we might be inside a 
> function template specialization or variable template specialization instead, 
> in some weird cases:
> 
> ```
> template concept C = true;
> template auto L = [] U>() {};
> struct Q {
>   template U> friend constexpr auto decltype(L)::operator()() 
> const;
> };
> ```
> 
> ... so I think we want a different approach than looking for an enclosing 
> class template specialization declaration.
> 
> Can we skip this check entirely, and instead always compute and substitute 
> the template instantiation arguments as is done below? That computation will 
> walk the enclosing contexts for us in a careful way that properly handles 
> cases like this lambda-in-variable-template situation. If we find we get zero 
> levels of template argument list, we can skip doing the actual substitution 
> as an optimization.
Sorry, that should be:
```
template concept C = true;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-03 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

Can you also change www/cxx_status.html to list P2103R0 as supported in the 
most recent version of Clang rather than in some previous version?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-04-03 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.

Comment at: clang/lib/Sema/SemaConcept.cpp:775-776
+static bool IsInsideImplicitFullTemplateSpecialization(const DeclContext *DC) {
+  auto *CTSDecl = dyn_cast_or_null(
+  DC->getOuterLexicalRecordContext());
+  return CTSDecl && !isa(CTSDecl) &&

This doesn't look right to me; there could be a class template nested inside a 
non-templated class, so I think you would need to walk up the enclosing 
`DeclContext`s one by one checking each in turn. But, we might be inside a 
function template specialization or variable template specialization instead, 
in some weird cases:

```
template concept C = true;
template auto L = [] U>() {};
struct Q {
  template U> friend constexpr auto decltype(L)::operator()() const;
};
```

... so I think we want a different approach than looking for an enclosing class 
template specialization declaration.

Can we skip this check entirely, and instead always compute and substitute the 
template instantiation arguments as is done below? That computation will walk 
the enclosing contexts for us in a careful way that properly handles cases like 
this lambda-in-variable-template situation. If we find we get zero levels of 
template argument list, we can skip doing the actual substitution as an 
optimization.

Comment at: clang/lib/Sema/SemaOverload.cpp:1303
+OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch,
+SourceLocation(), false /* PartialOrdering */);
 bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnType(),

rsmith wrote:
> shafik wrote:
> > nit
> Just remove the final parameter; it has a default argument of `false` and no 
> other call site passes `false` here.  (I'm working on removing this parameter 
> in a different change.)
You can remove the `SourceLocation()` argument too; there's an identical 
default argument.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-31 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@rsmith - thanks a lot for the review, is there anything you'd like me to do on 
this diff or we are good to go ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-31 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 509910.
alexander-shaposhnikov added a comment.

Rebased + rerun all the tests + internal testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp

Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,153 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.
+static_assert(S<4>::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_member_sfinae
+
+namespace requires_expression_references_members {
+
+void accept1(int x);
+void accept2(XY xy);
+
+template  struct S {
+  T Field = T();
+
+  constexpr int constrained_method()
+  requires requires { accept1(Field); };
+
+  constexpr int constrained_method()
+  requires requires { accept2(Field); };
+};
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept1(Field); } {
+  return CONSTRAINED_METHOD_1;
+}
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept2(Field); } {
+  return CONSTRAINED_METHOD_2;
+}
+
+static_assert(S().constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S().constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace requires_expression_references_members
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1653,33 +1653,12 @@
 << QualifierLoc.getSourceRange();
   return nullptr;
 }
-
-if (PrevClassTemplate) {
-  const ClassTemplateDecl *MostRecentPrevCT =
-

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-29 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 509546.
alexander-shaposhnikov added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp

Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,153 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.
+static_assert(S<4>::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_member_sfinae
+
+namespace requires_expression_references_members {
+
+void accept1(int x);
+void accept2(XY xy);
+
+template  struct S {
+  T Field = T();
+
+  constexpr int constrained_method()
+  requires requires { accept1(Field); };
+
+  constexpr int constrained_method()
+  requires requires { accept2(Field); };
+};
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept1(Field); } {
+  return CONSTRAINED_METHOD_1;
+}
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept2(Field); } {
+  return CONSTRAINED_METHOD_2;
+}
+
+static_assert(S().constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S().constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace requires_expression_references_members
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1653,33 +1653,12 @@
 << QualifierLoc.getSourceRange();
   return nullptr;
 }
-
-if (PrevClassTemplate) {
-  const ClassTemplateDecl *MostRecentPrevCT =
-  PrevClassTemplate->getMostRecentDecl();
-

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-29 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

This all looks good to me. Sorry for the delay.




Comment at: clang/lib/Sema/SemaConcept.cpp:728
  bool SkipForSpecialization = false) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
   ND, /*Final=*/false, /*Innermost=*/nullptr, /*RelativeToPrimary=*/true,

erichkeane wrote:
> Note all uses of THIS function are probably wrong based on what Richard says? 
>  I think even the friend-constraint-depends-on-enclosing-template thing is 
> wrong?  Is that right @rsmith ?
I've looked through them and yeah, I think they should all be replaced by 
something else -- though that seems less urgent than this change. One important 
goal should be removal of `AdjustConstraintDepth` and 
`ConstraintRefersToContainingTemplateChecker` -- a `TreeTransform` subclass is 
*hugely* expensive in terms of code size, and we don't need one here, let alone 
two.

Specifically:

- `AreConstraintExpressionsEqual` we've already discussed in this PR; this code 
is wrong.

- In `FriendConstraintDependOnEnclosingTemplate`, the depth calculation seems 
fine, but `ConstraintExpressionDependsOnEnclosingTemplate` uses a 
`TreeTransform` just to see if the original expression mentioned any enclosing 
template parameters, which is really inefficient and expensive. We should 
change that to use `Sema::MarkUsedTemplateParameters` instead. It has a 
`RecursiveASTVisitor` that computes this. It would need some changes to add a 
mode to detect template parameters at <= depth instead of at == depth, or we 
could call it in a loop I suppose.

- In `IsAtLeastAsConstrained`, it's really not clear what the intended language 
behavior is when the constraints have different template depths. I think there 
are two interesting cases here:
- The case where one or both functions are friends.
- The case where one function is a member and the other is a non-member, 
during operator lookup.
It's not really obvious what the right behavior is in either case. What we're 
currently doing doesn't make a lot of sense -- unrelated enclosing template 
parameters from different templates will get compared and considered equivalent 
-- but I don't have much of a better answer. Perhaps the thing that makes the 
most sense is to perform a substitution (`SubstExpr`) for non-member-like 
constrained friends (see https://reviews.llvm.org/D147068 and 
https://eel.is/c++draft/temp.friend#9 for context) and consider any other case 
where the functions were declared in different template scopes to have no 
ordering by constraints.

I do wonder whether, in the case of a non-member-like constrained friend 
function template, we should actually do the depth adjustment as part of 
instantiation (that is, substitute into the constraint in that case). That 
would make the rest of the handling simpler: no depth adjustment would be 
necessary in `IsAtLeastAsConstrained`.



Comment at: clang/lib/Sema/SemaOverload.cpp:1303
+OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch,
+SourceLocation(), false /* PartialOrdering */);
 bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnType(),

shafik wrote:
> nit
Just remove the final parameter; it has a default argument of `false` and no 
other call site passes `false` here.  (I'm working on removing this parameter 
in a different change.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-23 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D146178#4214972 , 
@alexander-shaposhnikov wrote:

> Add more tests.
> P.S. we already have tests with self-friends (in concepts.cpp), the test from 
> Richard's comment is also included (struct S12) (in a slightly simplified 
> form)

Interesting, that self-friend version did NOT fail for me (see the change I 
made which was much less involved).  I think on this one I'm unable to review 
it without Richard, so hopefully he can come back and give comments.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 507560.
alexander-shaposhnikov added a comment.

Add more tests.
P.S. we already have tests with self-friends (in concepts.cpp), the test from 
Richard's comment is also included (struct S12) (in a slightly simplified form)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp

Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,153 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.
+static_assert(S<4>::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_member_sfinae
+
+namespace requires_expression_references_members {
+
+void accept1(int x);
+void accept2(XY xy);
+
+template  struct S {
+  T Field = T();
+
+  constexpr int constrained_method()
+  requires requires { accept1(Field); };
+
+  constexpr int constrained_method()
+  requires requires { accept2(Field); };
+};
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept1(Field); } {
+  return CONSTRAINED_METHOD_1;
+}
+
+template 
+constexpr int S::constrained_method()
+  requires requires { accept2(Field); } {
+  return CONSTRAINED_METHOD_2;
+}
+
+static_assert(S().constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S().constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace requires_expression_references_members
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1653,33 +1653,12 @@
 << QualifierLoc.getSourceRange();

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov planned changes to this revision.
alexander-shaposhnikov added a comment.

will add more tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1676
+Inst->setLexicalDeclContext(Owner);
+RecordInst->setLexicalDeclContext(Owner);
+

this bit is important


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D146178#4213943 , 
@alexander-shaposhnikov wrote:

> @erichkeane - thanks for the comments, the changes in 
> SemaTemplateInstantiateDecl.cpp are necessary, in particular, they enable us 
> to handle the case
>
>   template 
>   concept Constraint = true;
>   
>   template 
>   struct Iterator {
> template 
> friend class Iterator;
>   };
>   
>   Iterator I;
>
> (and the negative one) 
> with the same machinery.
> Regarding SFINAE  - it's necessary as well, 
> the failed substitution is considered as "not equal" (according to the 
> standard (mentioned by Richard above)).

Yeah, I think you're right about the SFINAE bit.  I'll have to think on that 
example a while.  Hopefully Richard can stop by and comment on this instead, 
I'm not sure I completely get the how those changes work.  One thing I DID end 
up doing that I forgot to mention, was :

  --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
  +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
  @@ -208,6 +208,10 @@ Response HandleFunction(const FunctionDecl *Function,
 return Response::UseNextDecl(Function);
   }
  
  +Response HandleFunctionTemplateDecl(const FunctionTemplateDecl *FTD) {
  +  return Response::ChangeDecl(FTD->getLexicalDeclContext());
  +}
  +
   Response HandleRecordDecl(const CXXRecordDecl *Rec,
 MultiLevelTemplateArgumentList ,
 ASTContext ,
  @@ -318,6 +322,8 @@ MultiLevelTemplateArgumentList 
Sema::getTemplateInstantiationArgs(
   } else if (const auto *CSD =
  dyn_cast(CurDecl)) {
 R = HandleImplicitConceptSpecializationDecl(CSD, Result);
  +} else if (const auto *FTD = dyn_cast(CurDecl)) {
  +  R = HandleFunctionTemplateDecl(FTD);
   } else if (!isa(CurDecl)) {
 R = Response::DontClearRelativeToPrimaryNextDecl(CurDecl);
 if (CurDecl->getDeclContext()->isTranslationUnit()) {
  @@ -3949,16 +3955,16 @@ Sema::SubstExpr(Expr *E, const 
MultiLevelTemplateArgumentList ) {
   }

Which was necessary for properly getting the template argument lists for the 
out of line version, so I'm a little shocked/surprised it worked without it 
(else, the MLTAL for an out of line definition will include the struct's 
template arguments, which seems wrong).

I think the example you included there needs to go into the tests, as does the 
one that Richard provided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@erichkeane - thanks for the comments, the changes in 
SemaTemplateInstantiateDecl.cpp are necessary, in particular, they enable us to 
handle the case

  template 
  concept Constraint = true;
  
  template 
  struct Iterator {
template 
friend class Iterator;
  };
  
  Iterator I;

(and the negative one) 
with the same machinery.
Regarding SFINAE  - it's necessary as well, 
the failed substitution is considered as "not equal" (according to the standard 
(mentioned by Richard above)).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

Hmm... there is a lot going on here that I'm not sure is necessary? I was able 
to change the `AreConstraintExpressionsEqual` 'if' body to just:

  +MultiLevelTemplateArgumentList OldMLTAL =
  +getTemplateArgumentsForComparison(*this, Old);
  +MultiLevelTemplateArgumentList NewMLTAL =
  +getTemplateArgumentsForComparison(*this, New); // just does what you 
did to get the MLTAL, but in a separate function.
  +
  +ExprResult OldConstrRes = SubstConstraintExpr(OldConstr, OldMLTAL);
  +ExprResult NewConstrRes = SubstConstraintExpr(NewConstr, NewMLTAL);
  +
  +if (!OldConstrRes.isUsable() || !NewConstrRes.isUsable())
  +  return false;
  +
  +OldConstr = OldConstrRes.get();
  +NewConstr = NewConstrRes.get();

and the changes to SemaTemplateInstantiateDecl don't seem necessary?  Though I 
see the SFINAE trap is perhaps a good idea.

With that all done, the it seemed the only problem was with how a requires 
clause was instantiated(which I suspect needs to just not check the constraint 
always). But I don't see the rest of this patch is relevant?  I DO note that 
with the change above, your test all works.




Comment at: clang/lib/Sema/SemaConcept.cpp:744
 namespace {
   class AdjustConstraintDepth : public TreeTransform {
   unsigned TemplateDepth = 0;

This class here should probably go away now, since we're replacing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov updated this revision to Diff 507432.
alexander-shaposhnikov edited the summary of this revision.
alexander-shaposhnikov added a comment.

New approach to constraints' comparison


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

Files:
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaTemplate/concepts-out-of-line-def.cpp

Index: clang/test/SemaTemplate/concepts-out-of-line-def.cpp
===
--- clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -127,3 +127,121 @@
 static_assert(S::specialization("str") == SPECIALIZATION_REQUIRES);
 
 } // namespace multiple_template_parameter_lists
+
+static constexpr int CONSTRAINED_METHOD_1 = 1;
+static constexpr int CONSTRAINED_METHOD_2 = 2;
+
+namespace constrained_members {
+
+template 
+struct S {
+  template 
+  static constexpr int constrained_method();
+};
+
+template <>
+template 
+constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  template T4>
+  static constexpr int constrained_method();
+};
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained members
+
+namespace constrained_members_of_nested_types {
+
+template 
+struct S {
+  struct Inner0 {
+struct Inner1 {
+  template 
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template <>
+template 
+constexpr int S<1>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template <>
+template 
+constexpr int S<2>::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S<1>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S<2>::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+
+template 
+concept ConceptT1T2 = true;
+
+template
+struct S12 {
+  struct Inner0 {
+struct Inner1 {
+  template T4>
+  static constexpr int constrained_method();
+};
+  };
+};
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_1; }
+
+template<>
+template T5>
+constexpr int S12::Inner0::Inner1::constrained_method() { return CONSTRAINED_METHOD_2; }
+
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_1);
+static_assert(S12::Inner0::Inner1::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_members_of_nested_types
+
+namespace constrained_member_sfinae {
+
+template struct S {
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N * 1073741824 + 4]) == 16) {
+return CONSTRAINED_METHOD_1;
+  }
+
+  template
+  static constexpr int constrained_method() requires (sizeof(int[N]) == 16);
+};
+
+template<>
+template
+constexpr int S<4>::constrained_method() requires (sizeof(int[4]) == 16) {
+  return CONSTRAINED_METHOD_2;
+}
+
+// Verify that there is no amiguity in this case.
+static_assert(S<4>::constrained_method() == CONSTRAINED_METHOD_2);
+
+} // namespace constrained_member_sfinae
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1653,33 +1653,12 @@
 << QualifierLoc.getSourceRange();
   return nullptr;
 }
-
-if (PrevClassTemplate) {
-  const ClassTemplateDecl *MostRecentPrevCT =
-  PrevClassTemplate->getMostRecentDecl();
-  TemplateParameterList *PrevParams =
-  MostRecentPrevCT->getTemplateParameters();
-
-  // Make sure the parameter lists match.
-  if (!SemaRef.TemplateParameterListsAreEqual(
-  D->getTemplatedDecl(), InstParams,
-  MostRecentPrevCT->getTemplatedDecl(), PrevParams, true,
-  Sema::TPL_TemplateMatch))
-return nullptr;
-
-  // Do some additional validation, then merge default arguments
-  // from the existing declarations.
-  if (SemaRef.CheckTemplateParameterList(InstParams, PrevParams,
- Sema::TPC_ClassTemplate))
-return nullptr;
-}
   }

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:728
  bool SkipForSpecialization = false) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
   ND, /*Final=*/false, /*Innermost=*/nullptr, /*RelativeToPrimary=*/true,

Note all uses of THIS function are probably wrong based on what Richard says?  
I think even the friend-constraint-depends-on-enclosing-template thing is 
wrong?  Is that right @rsmith ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D146178#4213235 , 
@alexander-shaposhnikov wrote:

> @erichkeane - yes, I'm working on it, I hope to have a new version ~soon 
> (within a few days).

Ok, thanks for the update then!  I'll do just enough looking into it to be able 
to review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@erichkeane - yes, I'm working on it, I hope to have a new version ~soon 
(within a few days).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-22 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

@alexander-shaposhnikov : Are you working on @rsmith 's suggestion?  I finally 
have time to look into it, but dont want to repeat effort if you're already 1/2 
way into it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-17 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

In D146178#4200821 , @rsmith wrote:

> The approach of attempting to adjust the depth here is not correct, and we 
> should rip this whole mechanism out and reimplement it.  Consider this 
> closely related testcase:
>
>   template
>   concept Concept = true;
>   
>   template
>   struct A
>   {
> template V>
> void C(V&& t);
>   };
>   
>   template<>
>   template V>
>   void A::C(V&& t)
>   {}
>
> Clang rejects this because it's trying to compare `Concept` from the 
> declaration against `Concept` from the explicit specialization.
>
> The right way to fix that and the issue being addressed here is that, rather 
> than adjusting the depths, we should substitute the outer template arguments 
> from the scope specifier (`A::`) into the constraint before performing 
> the comparison. (In the special case where none of the outer template 
> parameters are used by the inner template, that does effectively just adjust 
> the depths of any inner template parameters.)
>
> The relevant language rule was introduced by the resolution of CA104 in 
> P2103R0 
>  
> (which Clang's status page incorrectly marks as complete in Clang 10). In 
> particular, see temp.constr.decl/4 
>  for the rule that we should 
> implement, but don't:
>
>> When determining whether a given introduced constraint-expression `C1` of a 
>> declaration in an instantiated specialization of a templated class is 
>> equivalent ([temp.over.link]) to the corresponding constraint-expression 
>> `C2` of a declaration outside the class body, `C1` is instantiated. If the 
>> instantiation results in an invalid expression, the constraint-expressions 
>> are not equivalent.
>
> So, in this case, we're supposed to instantiate the constraint expression 
> `Concept`, substituting the template arguments of `A` into it (in a SFINAE 
> context).

IIRC, the original motivation for this attempt was that we frequently compare 
template arguments, even in cases where that isn't permitted?  I had sent you a 
couple of emails when I was attempting this, but you'd not gotten a chance to 
respond.  I think I came up on this as the only way to do this without 
instantiating the constraint (which I recall you'd said was not the right way 
to compare constraints at one point?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-16 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

The approach of attempting to adjust the depth here is not correct, and we 
should rip this whole mechanism out and reimplement it.  Consider this closely 
related testcase:

  template
  concept Concept = true;
  
  template
  struct A
  {
template V>
void C(V&& t);
  };
  
  template<>
  template V>
  void A::C(V&& t)
  {}

Clang rejects this because it's trying to compare `Concept` from the 
declaration against `Concept` from the explicit specialization.

The right way to fix that and the issue being addressed here is that, rather 
than adjusting the depths, we should substitute the outer template arguments 
from the scope specifier (`A::`) into the constraint before performing the 
comparison. (In the special case where none of the outer template parameters 
are used by the inner template, that does effectively just adjust the depths of 
any inner template parameters.)

The relevant language rule was introduced by the resolution of CA104 in P2103R0 
 
(which Clang's status page incorrectly marks as complete in Clang 10). In 
particular, see temp.constr.decl/4  
for the rule that we should implement, but don't:

> When determining whether a given introduced constraint-expression `C1` of a 
> declaration in an instantiated specialization of a templated class is 
> equivalent ([temp.over.link]) to the corresponding constraint-expression `C2` 
> of a declaration outside the class body, `C1` is instantiated. If the 
> instantiation results in an invalid expression, the constraint-expressions 
> are not equivalent.

So, in this case, we're supposed to instantiate the constraint expression 
`Concept`, substituting the template arguments of `A` into it (in a SFINAE 
context).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-16 Thread Alexander Shaposhnikov via Phabricator via cfe-commits

alexander-shaposhnikov added a comment.

@shafik - it's a bit early to review this patch, this was the first attempt to 
fix the issue related to the current behavior of  
clang::Sema::TemplateParameterListsAreEqual that causes Clang to mishandle 
out-of-line definitions involving constraints (and possibly is the root cause 
of a few other issues related to concepts).
It's still work-in-progress. What's happening is roughly the following: there 
is a mismatch of depths of types (TemplateTypeParmType) referenced by 
ConceptSpecializationExpr. 
E.g. for the code

  template 
  concept Concept = true;
  
  template
  struct a
  {
template
void c(T1&& t);
  };
  
  template<>
  template
  void a::c(T2&& t)
  {}



  lldb) p OldConstr->dump()
  ConceptSpecializationExpr 0x64bc4c50 '_Bool' Concept 0x64bc4608 
'Concept'
  |-ImplicitConceptSpecializationDecl 0x64bc4be0 
  | `-TemplateArgument type 'type-parameter-1-0'
  |   `-TemplateTypeParmType 0x64bc4b70 'type-parameter-1-0' dependent 
depth 1 index 0
  `-TemplateArgument type 'T1'
`-TemplateTypeParmType 0x64bc4ba0 'T1' dependent depth 1 index 0
  `-TemplateTypeParm 0x64bc4ac8 'T1'
  
  (lldb) p NewConstr->dump()
  ConceptSpecializationExpr 0x64bc5140 '_Bool' Concept 0x64bc4608 
'Concept'
  |-ImplicitConceptSpecializationDecl 0x64bc50d0 
  | `-TemplateArgument type 'type-parameter-0-0'
  |   `-TemplateTypeParmType 0x64bc4580 'type-parameter-0-0' dependent 
depth 0 index 0
  `-TemplateArgument type 'T2'
`-TemplateTypeParmType 0x64bc5090 'T2' dependent depth 0 index 0
  `-TemplateTypeParm 0x64bc4ff0 'T2'

Inside Sema::AreConstraintExpressionsEqual there is some logic do adjust the 
depths, but at the moment i can't claim that i fully understand it - still 
investigating.
I think @erichkeane has also looked into the issue / debugged what's going on 
here - if I'm missing something / or something is not right - any corrections 
would be appreciated.
My current plan is to try to adopt the suggestion from

In D146178#4199382 , @erichkeane 
wrote:

> In D146178#4199263 , @erichkeane 
> wrote:
>
>> After a night of sleeping on it, the use of a AdjustConstraintDepth::Diff 
>> and AdjustConstraintDepth::Value feels like a hacky solution to SOMETHING 
>> here, though I'm not sure what yet. The depth of a template shouldn't change 
>> between the equality and constraint checking, it is usually a property of 
>> the specialization level.  I could definitely believe that 
>> `getTemplateInstantiationArgs` needs some sort of change here to better 
>> detect its own state, but this solution doesn't seem right to me.
>
> I debugged a bit: It isn't correct I think that `FunctionTemplateDecl` 
> doesn't pick up its template arguments in `getTemplateInstantiationArgs`.  
> Additionally, instead of picking up its `DeclContext`, it probably needs its 
> `LexicalDeclContext` as the next step, which I believe fixes the problem 
> (plus your change in SemaOverload.cpp)
>
> EDIT: `FunctionTemplateDecl` doesn't pick up its template arguments on 
> purpose: there ARE no template arguments, so that is me being silly.  
> However, the difference here is that it isn't picking its lexical decl 
> context here.  There is likely a similar solution here for VarTemplateDecl.
>
> I'm leaning toward the solution here being in the CalculateTemplateDepth here 
> instead: Recognize those two types, and set the correct DeclContext to pass 
> to the getTemplateInstantiationArgs (that is, the lexical decl context).

and see if i can make it functional, this will take some time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-16 Thread Shafik Yaghmour via Phabricator via cfe-commits

shafik added a comment.

I also don't get what is going on here, either a more detailed summary or more 
inline comments should help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

2023-03-16 Thread Shafik Yaghmour via Phabricator via cfe-commits

shafik added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:1303
+OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch,
+SourceLocation(), false /* PartialOrdering */);
 bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnType(),

nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

1 2 >

1 - 100 of 108 matches

Mail list logo