[Clang] Fix synthesis of defaulted operator==/<=> when class has an anonymous struct member #93380
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nonymous struct member
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Mital Ashok (MitalAshok) ChangesFixes #92497 The existing implementation did try to drill into anonymous structs and compare them member by member, but it did not properly build up the member access expressions to include the anonymous struct members. Note that this is different behaviour from GCC's anonymous struct extension where the defaulted comparison would compare Example of the difference: struct X {
struct {
bool b;
};
bool c;
template<typename T>
friend constexpr bool operator==(T& L, T& R) {
// With GCC, T is the type of the anonymous struct
// With Clang, this operator isn't used
// (L.b and R.b are compared directly in the below operator==)
static_assert(sizeof(T) == sizeof(bool));
return L.b == R.b;
}
friend constexpr bool operator==(const X& L, const X& R) = default;
};
static_assert(X{} == X{});This appears to be consistent with the Microsoft extension for anonymous structs Full diff: https://github.com/llvm/llvm-project/pull/93380.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d023f53754cb3..7985ab35439d2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -794,6 +794,8 @@ Bug Fixes to C++ Support
Fixes (#GH87210), (GH89541).
- Clang no longer tries to check if an expression is immediate-escalating in an unevaluated context.
Fixes (#GH91308).
+- Fix defaulted ``operator==``/``operator<=>`` not working properly with
+ classes that have anonymous struct members (#GH92497).
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 8ab429e2a136e..f573012ceacb9 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7948,7 +7948,13 @@ class DefaultedComparisonVisitor {
case DefaultedComparisonKind::Equal:
case DefaultedComparisonKind::ThreeWay:
+ assert(
+ AnonymousStructs.empty() &&
+ "Anonymous structs stack should be empty before visiting subobjects");
getDerived().visitSubobjects(Results, RD, ParamLvalType.getQualifiers());
+ assert(AnonymousStructs.empty() &&
+ "Anonymous structs stack should become empty again after visiting "
+ "subobjects");
return Results;
case DefaultedComparisonKind::NotEqual:
@@ -7985,6 +7991,7 @@ class DefaultedComparisonVisitor {
continue;
// Recursively expand anonymous structs.
if (Field->isAnonymousStructOrUnion()) {
+ AnonymousStructContextRAII R(*this, Field);
if (visitSubobjects(Results, Field->getType()->getAsCXXRecordDecl(),
Quals))
return true;
@@ -8027,6 +8034,30 @@ class DefaultedComparisonVisitor {
FunctionDecl *FD;
DefaultedComparisonKind DCK;
UnresolvedSet<16> Fns;
+ std::vector<FieldDecl *> AnonymousStructs;
+
+private:
+ struct AnonymousStructContextRAII {
+ DefaultedComparisonVisitor &Self;
+#ifndef NDEBUG
+ FieldDecl *FD;
+#endif
+ AnonymousStructContextRAII(AnonymousStructContextRAII &&) = delete;
+ explicit AnonymousStructContextRAII(DefaultedComparisonVisitor &Self,
+ FieldDecl *FD)
+ : Self(Self) {
+#ifndef NDEBUG
+ this->FD = FD;
+#endif
+ Self.AnonymousStructs.push_back(FD);
+ }
+ ~AnonymousStructContextRAII() {
+ assert(!Self.AnonymousStructs.empty() &&
+ Self.AnonymousStructs.back() == FD &&
+ "Invalid stack of anonymous structs");
+ Self.AnonymousStructs.pop_back();
+ }
+ };
};
/// Information about a defaulted comparison, as determined by
@@ -8535,6 +8566,8 @@ class DefaultedComparisonSynthesizer
}
ExprPair getBase(CXXBaseSpecifier *Base) {
+ assert(AnonymousStructs.empty() &&
+ "Visiting base class while inside an anonymous struct?");
ExprPair Obj = getCompleteObject();
if (Obj.first.isInvalid() || Obj.second.isInvalid())
return {ExprError(), ExprError()};
@@ -8550,12 +8583,30 @@ class DefaultedComparisonSynthesizer
if (Obj.first.isInvalid() || Obj.second.isInvalid())
return {ExprError(), ExprError()};
- DeclAccessPair Found = DeclAccessPair::make(Field, Field->getAccess());
- DeclarationNameInfo NameInfo(Field->getDeclName(), Loc);
- return {S.BuildFieldReferenceExpr(Obj.first.get(), /*IsArrow=*/false, Loc,
- CXXScopeSpec(), Field, Found, NameInfo),
- S.BuildFieldReferenceExpr(Obj.second.get(), /*IsArrow=*/false, Loc,
- CXXScopeSpec(), Field, Found, NameInfo)};
+ auto Reference = [&](FieldDecl *FD) -> bool {
+ DeclAccessPair Found = DeclAccessPair::make(FD, FD->getAccess());
+ DeclarationNameInfo NameInfo(FD->getDeclName(), Loc);
+
+ Obj.first =
+ S.BuildFieldReferenceExpr(Obj.first.get(), /*IsArrow=*/false, Loc,
+ CXXScopeSpec(), FD, Found, NameInfo);
+ Obj.second =
+ S.BuildFieldReferenceExpr(Obj.second.get(), /*IsArrow=*/false, Loc,
+ CXXScopeSpec(), FD, Found, NameInfo);
+ if (Obj.first.isInvalid() || Obj.second.isInvalid()) {
+ Obj.first = Obj.second = ExprError();
+ return true;
+ }
+ return false;
+ };
+
+ for (FieldDecl *AnonFD : AnonymousStructs) {
+ if (Reference(AnonFD))
+ return Obj;
+ }
+
+ Reference(Field);
+ return Obj;
}
// FIXME: When expanding a subobject, register a note in the code synthesis
diff --git a/clang/test/SemaCXX/anonymous-struct.cpp b/clang/test/SemaCXX/anonymous-struct.cpp
index e1db98d2b2f50..4418c40207f19 100644
--- a/clang/test/SemaCXX/anonymous-struct.cpp
+++ b/clang/test/SemaCXX/anonymous-struct.cpp
@@ -3,6 +3,18 @@
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++2a %s
+namespace std {
+#if __cplusplus >= 202002L
+ struct strong_ordering {
+ int n;
+ constexpr operator int() const { return n; }
+ static const strong_ordering less, equal, greater;
+ };
+ constexpr strong_ordering strong_ordering::less{-1},
+ strong_ordering::equal{0}, strong_ordering::greater{1};
+#endif
+}
+
struct S {
S();
#if __cplusplus <= 199711L
@@ -191,7 +203,7 @@ typedef struct { // expected-warning {{anonymous non-C-compatible type}}
} A; // expected-note {{given name 'A' for linkage purposes by this typedef}}
}
-#if __cplusplus > 201103L
+#if __cplusplus >= 201103L
namespace GH58800 {
struct A {
union {
@@ -207,3 +219,68 @@ A GetA() {
}
}
#endif
+
+#if __cplusplus >= 202002L
+namespace GH92497 {
+struct S {
+ struct {
+ bool b;
+ };
+
+ constexpr bool operator==(const S&) const noexcept;
+};
+constexpr bool S::operator==(const S&) const noexcept = default;
+bool f(S const& a, S const& b) { return a == b; }
+static_assert(S{} == S{});
+
+template<typename T>
+struct A {
+ struct {
+ T x;
+ };
+ friend constexpr bool operator==(const A&, const A&) = default;
+// expected-note@-1 2 {{candidate function has been implicitly deleted}}
+ friend constexpr bool operator<=>(const A&, const A&) = default;
+// expected-note@-1 2 {{candidate function has been implicitly deleted}}
+};
+
+static_assert(A<int>{} == A<int>{});
+static_assert(A<int>{} <= A<int>{});
+
+struct eq_but_not_cmp {
+ constexpr bool operator==(eq_but_not_cmp) const { return true; }
+};
+static_assert(A<eq_but_not_cmp>{} == A<eq_but_not_cmp>{});
+static_assert(A<eq_but_not_cmp>{} <=> A<eq_but_not_cmp>{});
+// expected-error@-1 {{overload resolution selected deleted operator '<=>'}}
+
+struct cmp_but_not_eq {
+ constexpr bool operator==(cmp_but_not_eq) = delete;
+ constexpr std::strong_ordering operator<=>(cmp_but_not_eq) const { return std::strong_ordering::equal; }
+};
+static_assert(A<cmp_but_not_eq>{} == A<cmp_but_not_eq>{});
+// expected-error@-1 {{overload resolution selected deleted operator '=='}}
+static_assert((A<cmp_but_not_eq>{} <=> A<cmp_but_not_eq>{}) == std::strong_ordering::equal);
+
+struct neither {};
+static_assert(A<neither>{} == A<neither>{});
+// expected-error@-1 {{overload resolution selected deleted operator '=='}}
+static_assert(A<neither>{} <=> A<neither>{});
+// expected-error@-1 {{overload resolution selected deleted operator '<=>'}}
+
+struct B {
+ struct {
+ struct {
+ struct {
+ bool b;
+ };
+ };
+ };
+ friend constexpr bool operator==(const B&, const B&) = default;
+ friend constexpr bool operator<=>(const B&, const B&) = default;
+};
+
+static_assert(B{} == B{});
+static_assert(B{} <= B{});
+}
+#endif
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #92497
The existing implementation did try to drill into anonymous structs and compare them member by member, but it did not properly build up the member access expressions to include the anonymous struct members.
Note that this is different behaviour from GCC's anonymous struct extension where the defaulted comparison would compare
LHS.<anonymous struct>withRHS.<anonymous struct>, which would fail if there is no overload for those types.Example of the difference:
This appears to be consistent with the Microsoft extension for anonymous structs