Skip to content

[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

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

AMP999
Copy link
Contributor

@AMP999 AMP999 commented Oct 8, 2023

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.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 8, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 8, 2023

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/68506.diff

2 Files Affected:

  • (modified) clang/lib/AST/Type.cpp (+2)
  • (modified) clang/test/SemaCXX/type-traits.cpp (+21-3)
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();

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@AMP999 AMP999 force-pushed the pr2-lambda-equality_comparable branch 2 times, most recently from a584b4b to d703c4a Compare October 8, 2023 14:41
Copy link
Contributor

@cor3ntin cor3ntin left a 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

Comment on lines 2666 to 2668
if (Decl->isLambda())
return Decl->captures().empty() &&
(Decl->getLambdaCaptureDefault() == LCD_None);
Copy link
Contributor

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

@AMP999 AMP999 force-pushed the pr2-lambda-equality_comparable branch from d703c4a to 896fbb6 Compare October 11, 2023 11:40
@llvmbot llvmbot added the clang:codegen IR generation bugs: mangling, exceptions, etc. label Oct 11, 2023
@AMP999 AMP999 force-pushed the pr2-lambda-equality_comparable branch 4 times, most recently from edd82ed to 344455e Compare October 11, 2023 11:58
@AMP999
Copy link
Contributor Author

AMP999 commented Oct 11, 2023

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

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 operator() of a lambda with captures under -fsanitize=null, but I couldn't see any null-check in the generated code even when the lambda did have captures, so I'm not sure I understand it. That codepath is
@zygoloid's from 2017 (376c28e). Should I keep that diff, or revert it?

@cor3ntin
Copy link
Contributor

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.
@AMP999 AMP999 force-pushed the pr2-lambda-equality_comparable branch from 344455e to baeace1 Compare October 11, 2023 14:45
@AMP999
Copy link
Contributor Author

AMP999 commented Oct 11, 2023

@cor3ntin Can you land this for me?

@cor3ntin cor3ntin merged commit 4313351 into llvm:main Oct 11, 2023
@AMP999 AMP999 deleted the pr2-lambda-equality_comparable branch October 11, 2023 17:25
@efriedma-quic
Copy link
Collaborator

efriedma-quic commented Oct 31, 2023

Do you need to check if the lambda is templated? For example, the following doesn't compile:

auto GetNonCapturingLambda() { return []<class T>(){ return 42; }; }
auto f() { return GetNonCapturingLambda() == GetNonCapturingLambda(); }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants