Skip to content

Commit

Permalink
[C++20] [Modules] Allow Stmt::Profile to treat lambdas as equal condi…
Browse files Browse the repository at this point in the history
…tionally

Close llvm/llvm-project#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
  • Loading branch information
ChuanqiXu9 authored and veselypeta committed Sep 4, 2024
2 parents 573a5a6 + 74258f4 commit b2b363a
Show file tree
Hide file tree
Showing 5 changed files with 230 additions and 24 deletions.
7 changes: 6 additions & 1 deletion clang/include/clang/AST/Stmt.h
Original file line number Diff line number Diff line change
Expand Up @@ -1298,8 +1298,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.
Expand Down
13 changes: 4 additions & 9 deletions clang/lib/AST/ASTContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6448,8 +6448,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;
}

Expand Down Expand Up @@ -6814,13 +6814,8 @@ bool ASTContext::isSameEntity(const NamedDecl *X, const NamedDecl *Y) const {
// ConceptDecl wouldn't be the same if their constraint expression differs.
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;
}

Expand Down
46 changes: 32 additions & 14 deletions clang/lib/AST/StmtProfile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -2034,14 +2039,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
Expand Down Expand Up @@ -2395,8 +2413,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);
}

Expand Down
100 changes: 100 additions & 0 deletions clang/test/Modules/merge-requires-with-lambdas.cppm
Original file line number Diff line number Diff line change
@@ -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'}}
88 changes: 88 additions & 0 deletions clang/test/Modules/pr63544.cppm
Original file line number Diff line number Diff line change
@@ -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;
}

0 comments on commit b2b363a

Please sign in to comment.