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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

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

> Thanks for the changes, they look good! While I was looking how we handle "requires" constraints in other cases, I've noticed that they are suspiciously similar. That's why I offer the following change to unify comparison of the constraint expressions
>
>   diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
>   index 92293622cc3d..555669f027a7 100644
>   --- a/clang/include/clang/AST/ASTContext.h
>   +++ b/clang/include/clang/AST/ASTContext.h
>   @@ -2664,6 +2664,11 @@ public:
>      /// that they may be used in declarations of the same template.
>      bool isSameTemplateParameter(const NamedDecl *X, const NamedDecl *Y) const;
>    
>   +  /// Determine whether two 'requires' expressions are similar enough.
>   +  /// Use of 'requires' isn't mandatory, works with constraints expressed in
>   +  /// other ways too.
>   +  bool isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const;
>   +
>      /// Retrieve the "canonical" template argument.
>      ///
>      /// The canonical template argument is the simplest template argument
>   diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
>   index aced0ab39ace..f3937d6304f9 100644
>   --- a/clang/lib/AST/ASTContext.cpp
>   +++ b/clang/lib/AST/ASTContext.cpp
>   @@ -6245,14 +6245,10 @@ bool ASTContext::isSameTemplateParameter(const NamedDecl *X,
>            auto *TYTCArgs = TYTC->getTemplateArgsAsWritten();
>            if (TXTCArgs->NumTemplateArgs != TYTCArgs->NumTemplateArgs)
>              return false;
>   -        llvm::FoldingSetNodeID XID, YID;
>   -        for (auto &ArgLoc : TXTCArgs->arguments())
>   -          ArgLoc.getArgument().Profile(XID, X->getASTContext());
>   -        for (auto &ArgLoc : TYTCArgs->arguments())
>   -          ArgLoc.getArgument().Profile(YID, Y->getASTContext());
>   -        if (XID != YID)
>   -          return false;
>          }
>   +      if (!isSameConstraintExpr(TXTC->getImmediatelyDeclaredConstraint(),
>   +                                TYTC->getImmediatelyDeclaredConstraint()))
>   +        return false;
>        }
>        return true;
>      }
>   @@ -6279,15 +6275,20 @@ bool ASTContext::isSameTemplateParameterList(
>        if (!isSameTemplateParameter(X->getParam(I), Y->getParam(I)))
>          return false;
>    
>   -  const Expr *XRC = X->getRequiresClause();
>   -  const Expr *YRC = Y->getRequiresClause();
>   -  if (!XRC != !YRC)
>   +  if (!isSameConstraintExpr(X->getRequiresClause(), Y->getRequiresClause()))
>        return false;
>   -  if (XRC) {
>   -    llvm::FoldingSetNodeID XRCID, YRCID;
>   -    XRC->Profile(XRCID, *this, /*Canonical=*/true);
>   -    YRC->Profile(YRCID, *this, /*Canonical=*/true);
>   -    if (XRCID != YRCID)
>   +
>   +  return true;
>   +}
>   +
>   +bool ASTContext::isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const {
>   +  if (!XCE != !YCE)
>   +    return false;
>   +  if (XCE) {
>   +    llvm::FoldingSetNodeID XCEID, YCEID;
>   +    XCE->Profile(XCEID, *this, /*Canonical=*/true);
>   +    YCE->Profile(YCEID, *this, /*Canonical=*/true);
>   +    if (XCEID != YCEID)
>          return false;
>      }
>    
>   @@ -6450,17 +6451,9 @@ bool ASTContext::isSameEntity(const NamedDecl *X, const NamedDecl *Y) const {
>            return false;
>        }
>    
>   -    const Expr *XRC = FuncX->getTrailingRequiresClause();
>   -    const Expr *YRC = FuncY->getTrailingRequiresClause();
>   -    if (!XRC != !YRC)
>   +    if (!isSameConstraintExpr(FuncX->getTrailingRequiresClause(),
>   +                              FuncY->getTrailingRequiresClause()))
>          return false;
>   -    if (XRC) {
>   -      llvm::FoldingSetNodeID XRCID, YRCID;
>   -      XRC->Profile(XRCID, *this, /*Canonical=*/true);
>   -      YRC->Profile(YRCID, *this, /*Canonical=*/true);
>   -      if (XRCID != YRCID)
>   -        return false;
>   -    }
>    
>        auto GetTypeAsWritten = [](const FunctionDecl *FD) {
>          // Map to the first declaration that we've already merged into this one.
>
> In `ASTContext::isSameTemplateParameter` it is exactly the same change as yours modulo comments.

Thanks for the detailed suggestion! Done!



================
Comment at: clang/test/Modules/concept.cppm:38
+  template <__integer_like _Tp, C<_Tp> Sentinel>
+  constexpr _Tp operator()(_Tp &&__t, Sentinel &&last) const {
+    return __t;
----------------
vsapsai wrote:
> In what cases `operator()` is critical for the test? I was thinking about replacing with something like "funcA", "funcB", "funcC", so that diagnostic verification is easier because it is tricky to understand which method "__fn::operator()" refers to.
The `operator()` is not critical for the test. It is reduced from the actual testing. But I prefer to remain `operator()` here. Since tests are rarely read and the current style could improve the diversity. Diversity is good for tests (to me).


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

https://reviews.llvm.org/D129068