-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[clang] __is_trivially_equality_comparable for types containing lambdas #68506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang ChangesLambdas (closure types) are trivially equality-comparable iff they are Full diff: https://github.com/llvm/llvm-project/pull/68506.diff 2 Files Affected:
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 4c433f7fe9daca0..f0e419de3b1ee84 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2663,6 +2663,8 @@ static bool
HasNonDeletedDefaultedEqualityComparison(const CXXRecordDecl *Decl) {
if (Decl->isUnion())
return false;
+ if (Decl->isLambda())
+ return Decl->captures().empty() && (Decl->getLambdaCaptureDefault() == LCD_None);
auto IsDefaultedOperatorEqualEqual = [&](const FunctionDecl *Function) {
return Function->getOverloadedOperator() ==
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index a35689d52978fcc..275ddcbae73930d 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -3160,11 +3160,18 @@ static_assert(!__is_trivially_equality_comparable(float), "");
static_assert(!__is_trivially_equality_comparable(double), "");
static_assert(!__is_trivially_equality_comparable(long double), "");
-struct TriviallyEqualityComparableNoDefaultedComparator {
+struct NonTriviallyEqualityComparableNoComparator {
int i;
int j;
};
-static_assert(!__is_trivially_equality_comparable(TriviallyEqualityComparableNoDefaultedComparator), "");
+static_assert(!__is_trivially_equality_comparable(NonTriviallyEqualityComparableNoComparator), "");
+
+struct NonTriviallyEqualityComparableNonDefaultedComparator {
+ int i;
+ int j;
+ bool operator==(const NonTriviallyEqualityComparableNonDefaultedComparator&);
+};
+static_assert(!__is_trivially_equality_comparable(NonTriviallyEqualityComparableNonDefaultedComparator), "");
#if __cplusplus >= 202002L
@@ -3177,7 +3184,7 @@ struct TriviallyEqualityComparable {
bool operator==(const TriviallyEqualityComparable&) const = default;
};
-static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparable), "");
+static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparable));
struct TriviallyEqualityComparableContainsArray {
int a[4];
@@ -3193,6 +3200,17 @@ struct TriviallyEqualityComparableContainsMultiDimensionArray {
};
static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparableContainsMultiDimensionArray));
+auto GetNonCapturingLambda() { return [](){ return 42; }; }
+
+struct TriviallyEqualityComparableContainsLambda {
+ [[no_unique_address]] decltype(GetNonCapturingLambda()) l;
+ int i;
+
+ bool operator==(const TriviallyEqualityComparableContainsLambda&) const = default;
+};
+static_assert(!__is_trivially_equality_comparable(decltype(GetNonCapturingLambda()))); // padding
+static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparableContainsLambda));
+
struct TriviallyEqualityComparableNonTriviallyCopyable {
TriviallyEqualityComparableNonTriviallyCopyable(const TriviallyEqualityComparableNonTriviallyCopyable&);
~TriviallyEqualityComparableNonTriviallyCopyable();
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
a584b4b
to
d703c4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is an opportunity for a small refactor here, but feel free to land this and we can handle the refactor later as an NFC change if you prefer
clang/lib/AST/Type.cpp
Outdated
if (Decl->isLambda()) | ||
return Decl->captures().empty() && | ||
(Decl->getLambdaCaptureDefault() == LCD_None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is used in a few places, maybe we should factor it out
d703c4a
to
896fbb6
Compare
edd82ed
to
344455e
Compare
Is this (344455e) what you had in mind? I only found a few places that could use the factored-out function, and in the "CodeGenFunction.cpp" case, I'm not even sure it applies. It looks like that codepath is intended to be hit when we codegen the |
Yup, this is what i have in mind. Afaik, the CodeGenFunction.cpp change was a bug that you fixed - just going off the comment. |
Lambdas (closure types) are trivially equality-comparable iff they are non-capturing, because non-capturing lambdas are convertible to function pointers: if `(lam1 == lam2)` compiles, then `lam1` and `lam2` must have the same type, and be always-equal, and be empty.
This changes the behavior of the call-site in "CodeGenFunction.cpp", I think for the better. But I couldn't figure out how to test that codepath.
344455e
to
baeace1
Compare
@cor3ntin Can you land this for me? |
Do you need to check if the lambda is templated? For example, the following doesn't compile:
|
Lambdas (closure types) are trivially equality-comparable iff they are
non-capturing, because non-capturing lambdas are convertible to function
pointers: if (lam1 == lam2) compiles, then lam1 and lam2 must have
the same type, and be always-equal, and be empty.