-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Clang] Partial implementation of support for P3074 (trivial unions) #146815
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-clang Author: Barry Revzin (brevzin) ChangesThis commit makes union construction/destruction trivial by default, but does not add the lifetime aspects of the paper — am holding out until P3726R0 will be discussed which alters that approach somewhat based on feedback from Richard Smith. But the constructor/destructor part is common regardless and also simpler. Full diff: https://github.com/llvm/llvm-project/pull/146815.diff 6 Files Affected:
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index ccb308e103253..49adbdf1c393d 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -1217,6 +1217,9 @@ void CXXRecordDecl::addedMember(Decl *D) {
// those because they are always unnamed.
bool IsZeroSize = Field->isZeroSize(Context);
+ // P3074
+ const bool TrivialUnion = Context.getLangOpts().CPlusPlus26 && isUnion();
+
if (const auto *RecordTy = T->getAs<RecordType>()) {
auto *FieldRec = cast<CXXRecordDecl>(RecordTy->getDecl());
if (FieldRec->getDefinition()) {
@@ -1277,7 +1280,7 @@ void CXXRecordDecl::addedMember(Decl *D) {
// -- for all the non-static data members of its class that are of
// class type (or array thereof), each such class has a trivial
// default constructor.
- if (!FieldRec->hasTrivialDefaultConstructor())
+ if (!FieldRec->hasTrivialDefaultConstructor() && !TrivialUnion)
data().HasTrivialSpecialMembers &= ~SMF_DefaultConstructor;
// C++0x [class.copy]p13:
@@ -1315,9 +1318,9 @@ void CXXRecordDecl::addedMember(Decl *D) {
if (!FieldRec->hasTrivialMoveAssignment())
data().HasTrivialSpecialMembers &= ~SMF_MoveAssignment;
- if (!FieldRec->hasTrivialDestructor())
+ if (!FieldRec->hasTrivialDestructor() && !TrivialUnion)
data().HasTrivialSpecialMembers &= ~SMF_Destructor;
- if (!FieldRec->hasTrivialDestructorForCall())
+ if (!FieldRec->hasTrivialDestructorForCall() && !TrivialUnion)
data().HasTrivialSpecialMembersForCall &= ~SMF_Destructor;
if (!FieldRec->hasIrrelevantDestructor())
data().HasIrrelevantDestructor = false;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index e8c65025bfe6d..e9b45bf99620c 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -9511,6 +9511,15 @@ bool SpecialMemberDeletionInfo::shouldDeleteForSubobjectCall(
CXXMethodDecl *Decl = SMOR.getMethod();
FieldDecl *Field = Subobj.dyn_cast<FieldDecl*>();
+ // P3074: default ctor and dtor for unions are not deleted, regardless of
+ // whether the underlying fields have non-trivial or deleted versions of those
+ // members
+ if (S.Context.getLangOpts().CPlusPlus26)
+ if (Field && Field->getParent()->isUnion() &&
+ (CSM == CXXSpecialMemberKind::DefaultConstructor ||
+ CSM == CXXSpecialMemberKind::Destructor))
+ return false;
+
int DiagKind = -1;
if (SMOR.getKind() == Sema::SpecialMemberOverloadResult::NoMemberOrDeleted)
@@ -9774,7 +9783,8 @@ bool SpecialMemberDeletionInfo::shouldDeleteForField(FieldDecl *FD) {
// At least one member in each anonymous union must be non-const
if (CSM == CXXSpecialMemberKind::DefaultConstructor &&
- AllVariantFieldsAreConst && !FieldRecord->field_empty()) {
+ AllVariantFieldsAreConst && !FieldRecord->field_empty() &&
+ !this->S.Context.getLangOpts().CPlusPlus26) {
if (Diagnose)
S.Diag(FieldRecord->getLocation(),
diag::note_deleted_default_ctor_all_const)
@@ -9804,6 +9814,10 @@ bool SpecialMemberDeletionInfo::shouldDeleteForAllConstMembers() {
// default constructor. Don't do that.
if (CSM == CXXSpecialMemberKind::DefaultConstructor && inUnion() &&
AllFieldsAreConst) {
+
+ if (S.Context.getLangOpts().CPlusPlus26)
+ return false;
+
bool AnyFields = false;
for (auto *F : MD->getParent()->fields())
if ((AnyFields = !F->isUnnamedBitField()))
diff --git a/clang/test/CXX/drs/cwg6xx.cpp b/clang/test/CXX/drs/cwg6xx.cpp
index e2eb009508b52..dd54bb5295b56 100644
--- a/clang/test/CXX/drs/cwg6xx.cpp
+++ b/clang/test/CXX/drs/cwg6xx.cpp
@@ -4,7 +4,7 @@
// RUN: %clang_cc1 -std=c++17 %s -verify=expected,cxx11-20,cxx98-17,cxx11-17,since-cxx11 -fexceptions -fcxx-exceptions -pedantic-errors
// RUN: %clang_cc1 -std=c++20 %s -verify=expected,cxx11-20,since-cxx11 -fexceptions -fcxx-exceptions -pedantic-errors
// RUN: %clang_cc1 -std=c++23 %s -verify=expected,since-cxx11 -fexceptions -fcxx-exceptions -pedantic-errors
-// RUN: %clang_cc1 -std=c++2c %s -verify=expected,since-cxx11 -fexceptions -fcxx-exceptions -pedantic-errors
+// RUN: %clang_cc1 -std=c++2c %s -verify=expected,since-cxx11,cxx26 -fexceptions -fcxx-exceptions -pedantic-errors
#if __cplusplus == 199711L
#define static_assert(...) __extension__ _Static_assert(__VA_ARGS__)
@@ -922,7 +922,7 @@ namespace cwg667 { // cwg667: 8
struct B { ~B() = delete; };
union C { B b; };
- static_assert(!__is_trivially_destructible(C), "");
+ static_assert(!__is_trivially_destructible(C), ""); // cxx26-error {{failed}}
struct D { D(const D&) = delete; };
struct E : D {};
diff --git a/clang/test/CXX/special/class.ctor/p5-0x.cpp b/clang/test/CXX/special/class.ctor/p5-0x.cpp
index e0c53058f892b..97a8b035a0705 100644
--- a/clang/test/CXX/special/class.ctor/p5-0x.cpp
+++ b/clang/test/CXX/special/class.ctor/p5-0x.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -Wno-deprecated-builtins -Wno-defaulted-function-deleted
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,until26 %s -std=c++11 -Wno-deprecated-builtins -Wno-defaulted-function-deleted
+// RUN: %clang_cc1 -fsyntax-only -verify=expected %s -std=c++26 -Wno-deprecated-builtins -Wno-defaulted-function-deleted
struct DefaultedDefCtor1 {};
struct DefaultedDefCtor2 { DefaultedDefCtor2() = default; };
@@ -23,8 +24,8 @@ int n;
// - X is a union-like class that has a variant member with a non-trivial
// default constructor,
-union Deleted1a { UserProvidedDefCtor u; }; // expected-note {{default constructor of 'Deleted1a' is implicitly deleted because variant field 'u' has a non-trivial default constructor}}
-Deleted1a d1a; // expected-error {{implicitly-deleted default constructor}}
+union Deleted1a { UserProvidedDefCtor u; }; // until26-note {{default constructor of 'Deleted1a' is implicitly deleted because variant field 'u' has a non-trivial default constructor}}
+Deleted1a d1a; // until26-error {{implicitly-deleted default constructor}}
union NotDeleted1a { DefaultedDefCtor1 nu; };
NotDeleted1a nd1a;
union NotDeleted1b { DefaultedDefCtor2 nu; };
@@ -86,19 +87,19 @@ NotDeleted3i nd3i;
union Deleted4a {
const int a;
const int b;
- const UserProvidedDefCtor c; // expected-note {{because variant field 'c' has a non-trivial default constructor}}
+ const UserProvidedDefCtor c; // until26-note {{because variant field 'c' has a non-trivial default constructor}}
};
-Deleted4a d4a; // expected-error {{implicitly-deleted default constructor}}
+Deleted4a d4a; // until26-error {{implicitly-deleted default constructor}}
union NotDeleted4a { const int a; int b; };
NotDeleted4a nd4a;
// - X is a non-union class and all members of any anonymous union member are of
// const-qualified type (or array thereof),
struct Deleted5a {
- union { const int a; }; // expected-note {{because all data members of an anonymous union member are const-qualified}}
+ union { const int a; }; // until26-note {{because all data members of an anonymous union member are const-qualified}}
union { int b; };
};
-Deleted5a d5a; // expected-error {{implicitly-deleted default constructor}}
+Deleted5a d5a; // until26-error {{implicitly-deleted default constructor}}
struct NotDeleted5a { union { const int a; int b; }; union { const int c; int d; }; };
NotDeleted5a nd5a;
diff --git a/clang/test/CXX/special/class.ctor/p6-0x.cpp b/clang/test/CXX/special/class.ctor/p6-0x.cpp
index 156a2b20c6b52..6ce1b06841ab4 100644
--- a/clang/test/CXX/special/class.ctor/p6-0x.cpp
+++ b/clang/test/CXX/special/class.ctor/p6-0x.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,until26 %s -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,cxx26 %s -std=c++26
// Implicitly-defined default constructors are constexpr if the implicit
// definition would be.
@@ -15,8 +16,9 @@ constexpr NonConstexpr2a nc2a = NonConstexpr2a(); // ok, does not call construct
constexpr int nc2_a = NonConstexpr2().nl.a; // ok
constexpr int nc2a_a = NonConstexpr2a().a; // ok
struct Helper {
- friend constexpr NonConstexpr1::NonConstexpr1(); // expected-error {{follows non-constexpr declaration}}
- friend constexpr NonConstexpr2::NonConstexpr2(); // expected-error {{follows non-constexpr declaration}}
+ friend constexpr NonConstexpr1::NonConstexpr1(); // until26-error {{follows non-constexpr declaration}} cxx26-error {{missing exception specification}}
+ friend constexpr NonConstexpr2::NonConstexpr2(); // until26-error {{follows non-constexpr declaration}} cxx26-error {{missing exception specification}}
+
};
struct Constexpr1 {};
@@ -31,14 +33,14 @@ constexpr Constexpr2 c2 = Constexpr2(); // ok
int n;
struct Member {
- Member() : a(n) {}
+ Member() : a(n) {} // cxx26-note {{here}}
constexpr Member(int&a) : a(a) {}
int &a;
};
-struct NonConstexpr4 { // expected-note {{here}}
+struct NonConstexpr4 { // until26-note {{here}} cxx26-note {{non-constexpr constructor}}
Member m;
};
-constexpr NonConstexpr4 nc4 = NonConstexpr4(); // expected-error {{constant expression}} expected-note {{non-constexpr constructor 'NonConstexpr4'}}
+constexpr NonConstexpr4 nc4 = NonConstexpr4(); // expected-error {{constant expression}} until26-note {{non-constexpr constructor 'NonConstexpr4'}} cxx26-note {{in call to}}
struct Constexpr3 {
constexpr Constexpr3() : m(n) {}
Member m;
@@ -53,11 +55,11 @@ constexpr Constexpr4 c4 = Constexpr4(); // ok
// This rule breaks some legal C++98 programs!
struct A {}; // expected-note {{here}}
struct B {
- friend A::A(); // expected-error {{non-constexpr declaration of 'A' follows constexpr declaration}}
+ friend A::A(); // until26-error {{non-constexpr declaration of 'A' follows constexpr declaration}} cxx26-error {{missing exception specification}}
};
namespace UnionCtors {
- union A { // expected-note {{here}}
+ union A { // until26-note {{here}}
int a;
int b;
};
@@ -79,7 +81,7 @@ namespace UnionCtors {
int d = 5;
};
};
- struct E { // expected-note {{here}}
+ struct E { // until26-note {{here}}
union {
int a;
int b;
@@ -87,11 +89,11 @@ namespace UnionCtors {
};
struct Test {
- friend constexpr A::A() noexcept; // expected-error {{follows non-constexpr declaration}}
+ friend constexpr A::A() noexcept; // until26-error {{follows non-constexpr declaration}}
friend constexpr B::B() noexcept;
friend constexpr C::C() noexcept;
friend constexpr D::D() noexcept;
- friend constexpr E::E() noexcept; // expected-error {{follows non-constexpr declaration}}
+ friend constexpr E::E() noexcept; // until26-error {{follows non-constexpr declaration}}
};
}
@@ -122,6 +124,6 @@ namespace PR48763 {
struct G { G(); };
struct H : D { using D::D; H(int); G g; };
- union V { H h; }; // expected-note {{field 'h' has a non-trivial default constructor}}
- V v; // expected-error {{deleted}}
+ union V { H h; }; // until26-note {{field 'h' has a non-trivial default constructor}}
+ V v; // until26-error {{deleted}}
}
diff --git a/clang/test/CXX/special/class.dtor/p5-0x.cpp b/clang/test/CXX/special/class.dtor/p5-0x.cpp
index ae14dcdaf102a..7616383515e0a 100644
--- a/clang/test/CXX/special/class.dtor/p5-0x.cpp
+++ b/clang/test/CXX/special/class.dtor/p5-0x.cpp
@@ -1,10 +1,11 @@
-// RUN: %clang_cc1 -verify -std=c++11 %s -Wno-defaulted-function-deleted -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -verify=expected,until26 -std=c++11 %s -Wno-defaulted-function-deleted -triple x86_64-linux-gnu
+// RUN: %clang_cc1 -verify=expected -std=c++26 %s -Wno-defaulted-function-deleted -triple x86_64-linux-gnu
struct NonTrivDtor {
~NonTrivDtor();
};
struct DeletedDtor {
- ~DeletedDtor() = delete; // expected-note 5 {{deleted here}}
+ ~DeletedDtor() = delete; // expected-note 4+ {{deleted here}}
};
class InaccessibleDtor {
~InaccessibleDtor() = default;
@@ -16,28 +17,28 @@ class InaccessibleDtor {
// destructor.
union A1 {
A1();
- NonTrivDtor n; // expected-note {{destructor of 'A1' is implicitly deleted because variant field 'n' has a non-trivial destructor}}
+ NonTrivDtor n; // until26-note {{destructor of 'A1' is implicitly deleted because variant field 'n' has a non-trivial destructor}}
};
-A1 a1; // expected-error {{deleted function}}
+A1 a1; // until26-error {{deleted function}}
struct A2 {
A2();
union {
- NonTrivDtor n; // expected-note {{because variant field 'n' has a non-trivial destructor}}
+ NonTrivDtor n; // until26-note {{because variant field 'n' has a non-trivial destructor}}
};
};
-A2 a2; // expected-error {{deleted function}}
+A2 a2; // until26-error {{deleted function}}
union A3 {
A3();
- NonTrivDtor n[3]; // expected-note {{because variant field 'n' has a non-trivial destructor}}
+ NonTrivDtor n[3]; // until26-note {{because variant field 'n' has a non-trivial destructor}}
};
-A3 a3; // expected-error {{deleted function}}
+A3 a3; // until26-error {{deleted function}}
struct A4 {
A4();
union {
- NonTrivDtor n[3]; // expected-note {{because variant field 'n' has a non-trivial destructor}}
+ NonTrivDtor n[3]; // until26-note {{because variant field 'n' has a non-trivial destructor}}
};
};
-A4 a4; // expected-error {{deleted function}}
+A4 a4; // until26-error {{deleted function}}
// -- any of the non-static data members has class type M (or array thereof) and
// M has a deleted or inaccessible destructor.
@@ -63,18 +64,18 @@ struct B4 {
B4 b4; // expected-error {{deleted function}}
union B5 {
B5();
- union { // expected-note-re {{because field 'B5::(anonymous union at {{.+}})' has a deleted destructor}}
- DeletedDtor a; // expected-note {{because field 'a' has a deleted destructor}}
+ union { // until26-note-re {{because field 'B5::(anonymous union at {{.+}})' has a deleted destructor}}
+ DeletedDtor a; // until26-note {{because field 'a' has a deleted destructor}}
};
};
-B5 b5; // expected-error {{deleted function}}
+B5 b5; // until26-error {{deleted function}}
union B6 {
B6();
- union { // expected-note-re {{because field 'B6::(anonymous union at {{.+}})' has a deleted destructor}}
- InaccessibleDtor a; // expected-note {{because field 'a' has an inaccessible destructor}}
+ union { // until26-note-re {{because field 'B6::(anonymous union at {{.+}})' has a deleted destructor}}
+ InaccessibleDtor a; // until26-note {{because field 'a' has an inaccessible destructor}}
};
};
-B6 b6; // expected-error {{deleted function}}
+B6 b6; // until26-error {{deleted function}}
// -- any direct or virtual base class has a deleted or inaccessible destructor.
struct C1 : DeletedDtor { C1(); } c1; // expected-error {{deleted function}} expected-note {{base class 'DeletedDtor' has a deleted destructor}}
|
| RecordDecl *Parent = Field->getParent(); | ||
| while (Parent && | ||
| (Parent->isAnonymousStructOrUnion() || | ||
| (Parent->isUnion() && Parent->getIdentifier() == nullptr))) { | ||
| if (auto RD = dyn_cast_or_null<RecordDecl>(Parent->getParent())) { | ||
| Parent = RD; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| auto ParentDecl = dyn_cast<CXXRecordDecl>(Parent); | ||
| if (!ParentDecl->isBeingDefined()) { | ||
| Sema::SpecialMemberOverloadResult SMOR = S.LookupSpecialMember( | ||
| ParentDecl, CXXSpecialMemberKind::DefaultConstructor, false, false, | ||
| false, false, false); | ||
| if (SMOR.getKind() == Sema::SpecialMemberOverloadResult::Success) { |
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'm guessing this is the wrong way to do this, but I don't know how to do it better.
Also this doesn't exactly implement the wording I quoted above because the wording is just wrong (see future CWG issue here).
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.
At least when it comes to getting the parent type, we do something similar in GetEnclosingNamedOrTopAnonRecord() in SemaBoundsSafety.cpp so I don’t think there is a helper for that.
CXXRecordDecl has hasTrivialDefaultConstructor() and friends; you might be able to make use of those here rather than performing a lookup manually (at least the check for !isBeingDefined() would imply to me that the flags that those functions query are accurate), but it’s possible I’m missing something here.
bcardosolopes
left a comment
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.
Added more reviewers and some nits
clang/lib/AST/DeclCXX.cpp
Outdated
| bool IsZeroSize = Field->isZeroSize(Context); | ||
|
|
||
| // P3074 | ||
| const bool TrivialUnion = Context.getLangOpts().CPlusPlus26 && isUnion(); |
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.
| const bool TrivialUnion = Context.getLangOpts().CPlusPlus26 && isUnion(); | |
| bool TrivialUnion = Context.getLangOpts().CPlusPlus26 && isUnion(); |
nit (we typically don’t use top-level const for local vars)
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.
Why not? I see a lot of them btw.
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 thought it was in our coding standards because I’m used to people pointing this out all the time but https://llvm.org/docs/CodingStandards.html doesn’t seem to have anything to say about it.
| struct Helper { | ||
| friend constexpr NonConstexpr1::NonConstexpr1(); // expected-error {{follows non-constexpr declaration}} | ||
| friend constexpr NonConstexpr2::NonConstexpr2(); // expected-error {{follows non-constexpr declaration}} | ||
| friend constexpr NonConstexpr1::NonConstexpr1(); // cxx11-23-error {{follows non-constexpr declaration}} cxx26-error {{missing exception specification}} |
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.
That ‘is missing exception specification 'noexcept'’ error is weird; I’m also getting that before C++26 and only Clang errors on this; GCC, MSVC, and EDG seem to be fine with it. I don’t think that has anything to do w/ this patch but it’s still strange.
| DeletedDtor a; // expected-note {{because field 'a' has a deleted destructor}} | ||
| }; | ||
| B1 b1; // expected-error {{deleted function}} | ||
| union B2 { |
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.
Some more cases:
union A {
struct {
NonTrivial x; // W/ and w/o a default member initialiser.
};
};
A a;
struct B {
union {
NonTrivial x; // as above
};
};
B b;
union {
struct {
union {
struct {
union {
NonTrivial x; // as above
};
};
};
};
} c;
struct {
union {
struct {
union {
struct {
NonTrivial x; // as above
};
};
};
};
} d;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.
Alright I'm not handling this case correctly, but I can't figure out why:
union U9 {
struct {
NonTrivial x = 1; // #6
};
} u9; // expected-error {{deleted function}}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.
Hmm, at a glance nothing jumps out to me; we do create a FieldDecl for the anonymous struct inside the union so I’d expect this to just handle that case—maybe we’re doing something weird with IndirectFieldDecls somewhere but that’s just a guess.
I did spot a fixme about anonymous unions inside anonymous unions, so er, here’s another gross test case I suppose:
union {
union {
NonTrivial x;
};
} a;
This commit makes union construction/destruction trivial by default, but does not add the lifetime aspects of the paper — am holding out until P3726R0 will be discussed which alters that approach somewhat based on feedback from Richard Smith. But the constructor/destructor part is common regardless and also simpler.