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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

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.



================
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;
----------------
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.


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

https://reviews.llvm.org/D129068