[PATCH] D129068: [AST] Profiling on constraint expression instead of arguments for TypeConstraint in ASTContext::isSameTemplateParameter

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

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.

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.

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).


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

https://reviews.llvm.org/D129068