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