[PATCH] D129068: [AST] Accept identical TypeConstraint referring to other template parameters.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D129068#3637716 <https://reviews.llvm.org/D129068#3637716>, @vsapsai wrote:

> After looking at this change more, I was thinking about changing the title from **how** to **what** you are doing. For example, something like "[AST] Accept identical TypeConstraint referring to other template parameters." You can tweak it as you know better what's going on but that is my general understanding of the change.

It looks better. Done.

> Instead of my previous ideas with `Profile` I'm looking into how we are handling constraints from different modules when they are specified in `requires` clause. Because I wasn't able to cause errors with
>
>   template <__integer_like _Tp, typename Sentinel>
>   constexpr _Tp funcA(_Tp &&__t, Sentinel &&last) requires C<Sentinel, _Tp> {
>     return __t;
>   }
>
> And it seems beneficial to handle constraints across modules in the same way. Similar story with template parameters referring to other template parameters.

For require clause, it is handled by https://github.com/llvm/llvm-project/blob/f6e0c05e3dcb0b6f28424fb3435d547c04c3edd5/clang/lib/AST/ASTContext.cpp#L6453-L6463. But sadly we couldn't follow its style directly. Since `requires` clause is different with the inplace concept usage in the AST level.

> In D129068#3629217 <https://reviews.llvm.org/D129068#3629217>, @ChuanqiXu wrote:
>
>>> And I have some ideas about the tests. It might be covered elsewhere but I'm curious if there are any not-isSameTemplateParameter test cases?
>>
>> I am not sure what you mean about "not-isSameTemplateParameter test case".
>
> For example, when I make the change
>
>   +#if 0
>            if (XID != YID)
>              return false;
>   +#endif
>
> in `ASTContext::isSameTemplateParameter`, only "PCH/cxx2a-constraints.cpp" is failing. So it looks like there aren't many tests verifying various similar-but-slightly-different constrained templates are rejected. I agree that some of such tests should have existed before your changes and I'm not asking to test all possible kinds of mismatches. But we can still bolster the test suite, I hope. Though specific tests might depend on the implementation, so you don't have to rush with extra tests (unless you want to).

Agreed. I added a test in this revision. And other kind of tests I imaged is covered by the checks in line 6230-6247. So I don't expect the current test covers all cases. But I feel it is not too bad.


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

https://reviews.llvm.org/D129068