[Clang] Make enums trivially equality comparable #133587
Conversation
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang/lib/Sema/SemaExprCXX.cpp clang/test/SemaCXX/type-traits.cppView the diff from clang-format here.diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index d4a9900d3..0b04996ff 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5202,7 +5202,8 @@ static bool EqualityComparisonIsDefaulted(Sema &S, const TypeDecl *Decl,
if (!Callee->isDefaulted())
return false;
if (!ParamT->isReferenceType()) {
- if (const CXXRecordDecl * RD = dyn_cast<CXXRecordDecl>(Decl); !RD->isTriviallyCopyable())
+ if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(Decl);
+ !RD->isTriviallyCopyable())
return false;
}
if (ParamT.getNonReferenceType()->getUnqualifiedDesugaredType() !=
|
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nikolas Klauser (philnik777) ChangesFixes #132672 Full diff: https://github.com/llvm/llvm-project/pull/133587.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 46895db4a0756..d4a9900d3fa8a 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5174,6 +5174,43 @@ static bool HasNoThrowOperator(const RecordType *RT, OverloadedOperatorKind Op,
return false;
}
+static bool EqualityComparisonIsDefaulted(Sema &S, const TypeDecl *Decl,
+ SourceLocation KeyLoc) {
+ EnterExpressionEvaluationContext UnevaluatedContext(
+ S, Sema::ExpressionEvaluationContext::Unevaluated);
+ Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
+ Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
+
+ // const ClassT& obj;
+ OpaqueValueExpr Operand(
+ KeyLoc, Decl->getTypeForDecl()->getCanonicalTypeUnqualified().withConst(),
+ ExprValueKind::VK_LValue);
+ UnresolvedSet<16> Functions;
+ // obj == obj;
+ S.LookupBinOp(S.TUScope, {}, BinaryOperatorKind::BO_EQ, Functions);
+
+ auto Result = S.CreateOverloadedBinOp(KeyLoc, BinaryOperatorKind::BO_EQ,
+ Functions, &Operand, &Operand);
+ if (Result.isInvalid() || SFINAE.hasErrorOccurred())
+ return false;
+
+ const auto *CallExpr = dyn_cast<CXXOperatorCallExpr>(Result.get());
+ if (!CallExpr)
+ return isa<EnumDecl>(Decl);
+ const auto *Callee = CallExpr->getDirectCallee();
+ auto ParamT = Callee->getParamDecl(0)->getType();
+ if (!Callee->isDefaulted())
+ return false;
+ if (!ParamT->isReferenceType()) {
+ if (const CXXRecordDecl * RD = dyn_cast<CXXRecordDecl>(Decl); !RD->isTriviallyCopyable())
+ return false;
+ }
+ if (ParamT.getNonReferenceType()->getUnqualifiedDesugaredType() !=
+ Decl->getTypeForDecl())
+ return false;
+ return true;
+}
+
static bool HasNonDeletedDefaultedEqualityComparison(Sema &S,
const CXXRecordDecl *Decl,
SourceLocation KeyLoc) {
@@ -5182,39 +5219,8 @@ static bool HasNonDeletedDefaultedEqualityComparison(Sema &S,
if (Decl->isLambda())
return Decl->isCapturelessLambda();
- {
- EnterExpressionEvaluationContext UnevaluatedContext(
- S, Sema::ExpressionEvaluationContext::Unevaluated);
- Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
- Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());
-
- // const ClassT& obj;
- OpaqueValueExpr Operand(
- KeyLoc,
- Decl->getTypeForDecl()->getCanonicalTypeUnqualified().withConst(),
- ExprValueKind::VK_LValue);
- UnresolvedSet<16> Functions;
- // obj == obj;
- S.LookupBinOp(S.TUScope, {}, BinaryOperatorKind::BO_EQ, Functions);
-
- auto Result = S.CreateOverloadedBinOp(KeyLoc, BinaryOperatorKind::BO_EQ,
- Functions, &Operand, &Operand);
- if (Result.isInvalid() || SFINAE.hasErrorOccurred())
- return false;
-
- const auto *CallExpr = dyn_cast<CXXOperatorCallExpr>(Result.get());
- if (!CallExpr)
- return false;
- const auto *Callee = CallExpr->getDirectCallee();
- auto ParamT = Callee->getParamDecl(0)->getType();
- if (!Callee->isDefaulted())
- return false;
- if (!ParamT->isReferenceType() && !Decl->isTriviallyCopyable())
- return false;
- if (ParamT.getNonReferenceType()->getUnqualifiedDesugaredType() !=
- Decl->getTypeForDecl())
- return false;
- }
+ if (!EqualityComparisonIsDefaulted(S, Decl, KeyLoc))
+ return false;
return llvm::all_of(Decl->bases(),
[&](const CXXBaseSpecifier &BS) {
@@ -5229,7 +5235,10 @@ static bool HasNonDeletedDefaultedEqualityComparison(Sema &S,
Type = Type->getBaseElementTypeUnsafe()
->getCanonicalTypeUnqualified();
- if (Type->isReferenceType() || Type->isEnumeralType())
+ if (Type->isReferenceType() ||
+ (Type->isEnumeralType() &&
+ !EqualityComparisonIsDefaulted(
+ S, cast<EnumDecl>(Type->getAsTagDecl()), KeyLoc)))
return false;
if (const auto *RD = Type->getAsCXXRecordDecl())
return HasNonDeletedDefaultedEqualityComparison(S, RD, KeyLoc);
@@ -5240,9 +5249,13 @@ static bool HasNonDeletedDefaultedEqualityComparison(Sema &S,
static bool isTriviallyEqualityComparableType(Sema &S, QualType Type, SourceLocation KeyLoc) {
QualType CanonicalType = Type.getCanonicalType();
if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType() ||
- CanonicalType->isEnumeralType() || CanonicalType->isArrayType())
+ CanonicalType->isArrayType())
return false;
+ if (CanonicalType->isEnumeralType())
+ return EqualityComparisonIsDefaulted(
+ S, cast<EnumDecl>(CanonicalType->getAsTagDecl()), KeyLoc);
+
if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
if (!HasNonDeletedDefaultedEqualityComparison(S, RD, KeyLoc))
return false;
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index b130024503101..657d5bcf07343 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -3873,6 +3873,11 @@ static_assert(!__is_trivially_equality_comparable(NonTriviallyEqualityComparable
#if __cplusplus >= 202002L
+enum TriviallyEqualityComparableEnum {
+ x, y
+};
+static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparableEnum));
+
struct TriviallyEqualityComparable {
int i;
int j;
@@ -3891,6 +3896,13 @@ struct TriviallyEqualityComparableContainsArray {
};
static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparableContainsArray));
+struct TriviallyEqualityComparableContainsEnum {
+ TriviallyEqualityComparableEnum e;
+
+ bool operator==(const TriviallyEqualityComparableContainsEnum&) const = default;
+};
+static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparableContainsEnum));
+
struct TriviallyEqualityComparableContainsMultiDimensionArray {
int a[4][4];
|
| x, y | ||
| }; | ||
| static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparableEnum)); | ||
|
|
There was a problem hiding this comment.
This is very nice, thank you!
At line 3853 add:
static_assert(__is_trivially_equality_comparable(Enum));
static_assert(__is_trivially_equality_comparable(SignedEnum));
static_assert(__is_trivially_equality_comparable(UnsignedEnum));
static_assert(__is_trivially_equality_comparable(EnumClass));
static_assert(__is_trivially_equality_comparable(SignedEnumClass));
static_assert(__is_trivially_equality_comparable(UnsignedEnumClass));
It would be beneficial also to test that the compiler does not crash in this case:
enum E { e };
static_assert(__is_trivially_equality_comparable(E));
bool operator==(E, E);
static_assert(!__is_trivially_equality_comparable(E));
There was a problem hiding this comment.
I'm not convinced that it's a good idea to test behaviour that's UB. AFAICT we don't have a test like this for any other trait.
There was a problem hiding this comment.
The summary should not merely link to an issue but describe the problem as well. Two reasons:
- for folks reading this in git log
- The issue may not totally explain what is going on.
I actually did not get it at first b/c the issue talks about char and std::byte it took me a bit to realize oh yes std::byte is an enum. A good summary would have clarified this for reviewers.
| return false; | ||
|
|
||
| if (CanonicalType->isEnumeralType()) | ||
| return EqualityComparisonIsDefaulted( |
There was a problem hiding this comment.
It looks like you are currently only testing if it is defaulted, we should be testing all code paths if possible to insure this does what we expect and to prevent future regressions.
There was a problem hiding this comment.
I'm not sure what you're referring to. If you mean I should test a non-trivially equality comparable enum, that's already there.
There was a problem hiding this comment.
I am probably being dense here but I thought for the both the test cases below EqualityComparisonIsDefaulted would return true so I was asking if there is a test case in which it returns false.
What else would you like? I've said in the title that I'm making enums trivially equality comparable. The bug just happens to be fixed by that - it's not like this fixes just that bug. |
Something like " |
|
@philnik777 Is anything else blocking this patch to fix #132672? Could you just change the commit message to what @shafik suggested and merge it please?
|
Fixes #132672