[clang] 74258f4 - [C++20] [Modules] Allow Stmt::Profile to treat lambdas as equal conditionally
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 11 01:18:29 PDT 2023
Author: Chuanqi Xu
Date: 2023-07-11T16:13:06+08:00
New Revision: 74258f4189e2b6bacabd40ee6f588fd9d1b37741
URL: https://github.com/llvm/llvm-project/commit/74258f4189e2b6bacabd40ee6f588fd9d1b37741
DIFF: https://github.com/llvm/llvm-project/commit/74258f4189e2b6bacabd40ee6f588fd9d1b37741.diff
LOG: [C++20] [Modules] Allow Stmt::Profile to treat lambdas as equal conditionally
Close https://github.com/llvm/llvm-project/issues/63544.
Background: We landed std modules in libcxx recently but we haven't
landed the corresponding in-tree tests. According to @Mordante, there
are only 1% libcxx tests failing with std modules. And the major
blocking issue is the lambda expression in the require clauses.
The root cause of the issue is that previously we never consider any
lambda expression as the same. Per [temp.over.link]p5:
> Two lambda-expressions are never considered equivalent.
I thought this is an oversight at first but @rsmith explains that in the
wording, the program is as if there is only a single definition, and a
single lambda-expression. So we don't need worry about this in the spec.
The explanation makes sense. But it didn't reflect to the implementation
directly.
Here is a cycle in the implementation. If we want to merge two
definitions, we need to make sure its implementation are the same. But
according to the explanation above, we need to judge if two
lambda-expression are the same by looking at its parent definitions. So
here is the problem.
To solve the problem, I think we have to profile the lambda expressions
actually to get the accurate information. But we can't do this
universally. So in this patch I tried to modify the interface of
`Stmt::Profile` and only profile the lambda expression during the
process of merging the constraint expressions.
Differential Revision: https://reviews.llvm.org/D153957
Added:
clang/test/Modules/merge-requires-with-lambdas.cppm
clang/test/Modules/pr63544.cppm
Modified:
clang/include/clang/AST/Stmt.h
clang/lib/AST/ASTContext.cpp
clang/lib/AST/StmtProfile.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 9a77f876c08ae7..156dd0a436a900 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -1297,8 +1297,13 @@ class alignas(void *) Stmt {
/// parameters are identified by index/level rather than their
/// declaration pointers) or the exact representation of the statement as
/// written in the source.
+ /// \param ProfileLambdaExpr whether or not to profile lambda expressions.
+ /// When false, the lambda expressions are never considered to be equal to
+ /// other lambda expressions. When true, the lambda expressions with the same
+ /// implementation will be considered to be the same. ProfileLambdaExpr should
+ /// only be true when we try to merge two declarations within modules.
void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
- bool Canonical) const;
+ bool Canonical, bool ProfileLambdaExpr = false) const;
/// Calculate a unique representation for a statement that is
/// stable across compiler invocations.
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 2b84673cf6b8fc..7acacd7bf4f504 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -6328,8 +6328,8 @@ bool ASTContext::isSameConstraintExpr(const Expr *XCE, const Expr *YCE) const {
return true;
llvm::FoldingSetNodeID XCEID, YCEID;
- XCE->Profile(XCEID, *this, /*Canonical=*/true);
- YCE->Profile(YCEID, *this, /*Canonical=*/true);
+ XCE->Profile(XCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true);
+ YCE->Profile(YCEID, *this, /*Canonical=*/true, /*ProfileLambdaExpr=*/true);
return XCEID == YCEID;
}
@@ -6694,13 +6694,8 @@ bool ASTContext::isSameEntity(const NamedDecl *X, const NamedDecl *Y) const {
// ConceptDecl wouldn't be the same if their constraint expression
diff ers.
if (const auto *ConceptX = dyn_cast<ConceptDecl>(X)) {
const auto *ConceptY = cast<ConceptDecl>(Y);
- const Expr *XCE = ConceptX->getConstraintExpr();
- const Expr *YCE = ConceptY->getConstraintExpr();
- assert(XCE && YCE && "ConceptDecl without constraint expression?");
- llvm::FoldingSetNodeID XID, YID;
- XCE->Profile(XID, *this, /*Canonical=*/true);
- YCE->Profile(YID, *this, /*Canonical=*/true);
- if (XID != YID)
+ if (!isSameConstraintExpr(ConceptX->getConstraintExpr(),
+ ConceptY->getConstraintExpr()))
return false;
}
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index a9c4e14e87eff3..307ad4925d36b7 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -29,10 +29,12 @@ namespace {
protected:
llvm::FoldingSetNodeID &ID;
bool Canonical;
+ bool ProfileLambdaExpr;
public:
- StmtProfiler(llvm::FoldingSetNodeID &ID, bool Canonical)
- : ID(ID), Canonical(Canonical) {}
+ StmtProfiler(llvm::FoldingSetNodeID &ID, bool Canonical,
+ bool ProfileLambdaExpr)
+ : ID(ID), Canonical(Canonical), ProfileLambdaExpr(ProfileLambdaExpr) {}
virtual ~StmtProfiler() {}
@@ -83,8 +85,10 @@ namespace {
public:
StmtProfilerWithPointers(llvm::FoldingSetNodeID &ID,
- const ASTContext &Context, bool Canonical)
- : StmtProfiler(ID, Canonical), Context(Context) {}
+ const ASTContext &Context, bool Canonical,
+ bool ProfileLambdaExpr)
+ : StmtProfiler(ID, Canonical, ProfileLambdaExpr), Context(Context) {}
+
private:
void HandleStmtClass(Stmt::StmtClass SC) override {
ID.AddInteger(SC);
@@ -181,7 +185,8 @@ namespace {
ODRHash &Hash;
public:
StmtProfilerWithoutPointers(llvm::FoldingSetNodeID &ID, ODRHash &Hash)
- : StmtProfiler(ID, false), Hash(Hash) {}
+ : StmtProfiler(ID, /*Canonical=*/false, /*ProfileLambdaExpr=*/false),
+ Hash(Hash) {}
private:
void HandleStmtClass(Stmt::StmtClass SC) override {
@@ -2030,14 +2035,27 @@ StmtProfiler::VisitCXXTemporaryObjectExpr(const CXXTemporaryObjectExpr *S) {
void
StmtProfiler::VisitLambdaExpr(const LambdaExpr *S) {
- // Do not recursively visit the children of this expression. Profiling the
- // body would result in unnecessary work, and is not safe to do during
- // deserialization.
- VisitStmtNoChildren(S);
+ if (!ProfileLambdaExpr) {
+ // Do not recursively visit the children of this expression. Profiling the
+ // body would result in unnecessary work, and is not safe to do during
+ // deserialization.
+ VisitStmtNoChildren(S);
+
+ // C++20 [temp.over.link]p5:
+ // Two lambda-expressions are never considered equivalent.
+ VisitDecl(S->getLambdaClass());
+
+ return;
+ }
- // C++20 [temp.over.link]p5:
- // Two lambda-expressions are never considered equivalent.
- VisitDecl(S->getLambdaClass());
+ CXXRecordDecl *Lambda = S->getLambdaClass();
+ ID.AddInteger(Lambda->getODRHash());
+
+ for (const auto &Capture : Lambda->captures()) {
+ ID.AddInteger(Capture.getCaptureKind());
+ if (Capture.capturesVariable())
+ VisitDecl(Capture.getCapturedVar());
+ }
}
void
@@ -2391,8 +2409,8 @@ void StmtProfiler::VisitTemplateArgument(const TemplateArgument &Arg) {
}
void Stmt::Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context,
- bool Canonical) const {
- StmtProfilerWithPointers Profiler(ID, Context, Canonical);
+ bool Canonical, bool ProfileLambdaExpr) const {
+ StmtProfilerWithPointers Profiler(ID, Context, Canonical, ProfileLambdaExpr);
Profiler.Visit(this);
}
diff --git a/clang/test/Modules/merge-requires-with-lambdas.cppm b/clang/test/Modules/merge-requires-with-lambdas.cppm
new file mode 100644
index 00000000000000..5767492047684b
--- /dev/null
+++ b/clang/test/Modules/merge-requires-with-lambdas.cppm
@@ -0,0 +1,100 @@
+// Tests that we can merge the concept declarations with lambda well.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 %t/A0.cppm -emit-module-interface -o %t/A0.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A1.cppm -emit-module-interface -o %t/A1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA1.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A2.cppm -emit-module-interface -o %t/A2.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA2.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// RUN: %clang_cc1 -std=c++20 %t/A3.cppm -emit-module-interface -o %t/A3.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TestA3.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+//--- A.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(const __Up&) {}(__t); };
+
+//--- A1.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(__Up) {}(__t); };
+
+//--- A2.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { []<class __Up>(const __Up& __u) {
+ (int)__u;
+}(__t); };
+
+//--- A3.h
+template <class _Tp>
+concept A = requires(const _Tp& __t) { [t = '?']<class __Up>(const __Up&) {
+ (int)t;
+}(__t); };
+
+//--- A.cppm
+module;
+#include "A.h"
+export module A;
+export using ::A;
+
+//--- A0.cppm
+module;
+#include "A.h"
+export module A0;
+export using ::A;
+
+//--- TestA.cpp
+// expected-no-diagnostics
+import A;
+import A0;
+
+template <class C>
+void f(C) requires(A<C>) {}
+
+//--- A1.cppm
+module;
+#include "A1.h"
+export module A1;
+export using ::A;
+
+//--- TestA1.cpp
+import A;
+import A1;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+ // expected-note@* 1+{{candidate found by name lookup is 'A'}}
+
+//--- A2.cppm
+module;
+#include "A2.h"
+export module A2;
+export using ::A;
+
+//--- TestA2.cpp
+import A;
+import A2;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+ // expected-note@* 1+{{candidate found by name lookup is 'A'}}
+
+//--- A3.cppm
+module;
+#include "A3.h"
+export module A3;
+export using ::A;
+
+//--- TestA3.cpp
+import A;
+import A3;
+
+template <class C>
+void f(C) requires(A<C>) {} // expected-error 1+{{reference to 'A' is ambiguous}}
+ // expected-note@* 1+{{candidate found by name lookup is 'A'}}
diff --git a/clang/test/Modules/pr63544.cppm b/clang/test/Modules/pr63544.cppm
new file mode 100644
index 00000000000000..16224cfd010949
--- /dev/null
+++ b/clang/test/Modules/pr63544.cppm
@@ -0,0 +1,88 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++23 %t/a.cppm -emit-module-interface -o %t/m-a.pcm
+// RUN: %clang_cc1 -std=c++23 %t/b.cppm -emit-module-interface -o %t/m-b.pcm
+// RUN: %clang_cc1 -std=c++23 %t/m.cppm -emit-module-interface -o %t/m.pcm \
+// RUN: -fprebuilt-module-path=%t
+// RUN: %clang_cc1 -std=c++23 %t/pr63544.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
+
+//--- foo.h
+
+namespace std {
+struct strong_ordering {
+ int n;
+ constexpr operator int() const { return n; }
+ static const strong_ordering equal, greater, less;
+};
+constexpr strong_ordering strong_ordering::equal = {0};
+constexpr strong_ordering strong_ordering::greater = {1};
+constexpr strong_ordering strong_ordering::less = {-1};
+} // namespace std
+
+namespace std {
+template <typename _Tp>
+class optional {
+private:
+ using value_type = _Tp;
+ value_type __val_;
+ bool __engaged_;
+public:
+ constexpr bool has_value() const noexcept
+ {
+ return this->__engaged_;
+ }
+
+ constexpr const value_type& operator*() const& noexcept
+ {
+ return __val_;
+ }
+
+ optional(_Tp v) : __val_(v) {
+ __engaged_ = true;
+ }
+};
+
+template <class _Tp>
+concept __is_derived_from_optional = requires(const _Tp& __t) { []<class __Up>(const optional<__Up>&) {}(__t); };
+
+template <class _Tp, class _Up>
+ requires(!__is_derived_from_optional<_Up>)
+constexpr strong_ordering
+operator<=>(const optional<_Tp>& __x, const _Up& __v) {
+ return __x.has_value() ? *__x <=> __v : strong_ordering::less;
+}
+} // namespace std
+
+//--- a.cppm
+module;
+#include "foo.h"
+export module m:a;
+export namespace std {
+ using std::optional;
+ using std::operator<=>;
+}
+
+//--- b.cppm
+module;
+#include "foo.h"
+export module m:b;
+export namespace std {
+ using std::optional;
+ using std::operator<=>;
+}
+
+//--- m.cppm
+export module m;
+export import :a;
+export import :b;
+
+//--- pr63544.cpp
+// expected-no-diagnostics
+import m;
+int pr63544() {
+ std::optional<int> a(43);
+ int t{3};
+ return a<=>t;
+}
More information about the cfe-commits
mailing list