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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
