From a26410c82f557f28fc5ac5dc9951a1e7601287c7 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Sun, 17 Nov 2024 20:50:41 +0000 Subject: [PATCH 01/10] add failing test Signed-off-by: Valentyn Yukhymenko --- .../reflection/define-aggregate.pass.cpp | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp index 1cba81402ccc83..532d0183aa0110 100644 --- a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp +++ b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp @@ -20,6 +20,7 @@ // // RUN: %{exec} %t.exe > %t.stdout +#include #include #include @@ -303,4 +304,28 @@ static_assert(is_type(define_aggregate(^^S2, { } // namespace repeat_calls +namespace non_trivial_constructor_and_destructor { +// https://github.com/bloomberg/clang-p2996/issues/115 +struct A { + constexpr A() { + // no-op + } + + constexpr ~A() { + // no-op + } +}; + +union U; +static_assert(is_type(define_aggregate( + ^^U, + { + data_member_spec(^^int), + data_member_spec(^^A), + }))); + +static_assert(std::is_trivially_constructible_v == true); +static_assert(std::is_trivially_destructible_v == true); +} // namespace non_trivial_constructor_and_destructor + int main() { } From 77f0d49b627f4bb03dc3f6faa780682b1e736791 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Mon, 18 Nov 2024 23:41:16 +0000 Subject: [PATCH 02/10] fix test since I misunderstood problem Signed-off-by: Valentyn Yukhymenko --- .../std/experimental/reflection/define-aggregate.pass.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp index 532d0183aa0110..b13656f3d611f4 100644 --- a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp +++ b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp @@ -324,8 +324,8 @@ static_assert(is_type(define_aggregate( data_member_spec(^^A), }))); -static_assert(std::is_trivially_constructible_v == true); -static_assert(std::is_trivially_destructible_v == true); +static_assert(std::is_default_constructible_v == true); +static_assert(std::is_destructible::value == true); } // namespace non_trivial_constructor_and_destructor int main() { } From 07eec9ee80c81d1f57239743b81bc15aa680d339 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Fri, 22 Nov 2024 01:46:36 +0000 Subject: [PATCH 03/10] better test Signed-off-by: Valentyn Yukhymenko --- .../experimental/reflection/define-aggregate.pass.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp index b13656f3d611f4..8390e69df5bc26 100644 --- a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp +++ b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp @@ -324,8 +324,14 @@ static_assert(is_type(define_aggregate( data_member_spec(^^A), }))); -static_assert(std::is_default_constructible_v == true); +static_assert(std::is_default_constructible::value == true); +static_assert(std::is_trivially_constructible::value == false); + static_assert(std::is_destructible::value == true); +static_assert(std::is_trivially_destructible::value == false); } // namespace non_trivial_constructor_and_destructor -int main() { } +int main() { + non_trivial_constructor_and_destructor::U u{1}; + u.~U(); +} From c71cdd57389d7fa4733c6224750a499ff0dc6c8e Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Sat, 23 Nov 2024 11:56:37 +0000 Subject: [PATCH 04/10] first version of fix Signed-off-by: Valentyn Yukhymenko --- clang/lib/AST/ExprConstantMeta.cpp | 18 +++++++++++++ clang/lib/Sema/SemaReflect.cpp | 17 ++++++++++++ .../reflection/define-aggregate.pass.cpp | 27 +++++++++++++++---- 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/clang/lib/AST/ExprConstantMeta.cpp b/clang/lib/AST/ExprConstantMeta.cpp index 9715682ff833de..919107d0aac215 100644 --- a/clang/lib/AST/ExprConstantMeta.cpp +++ b/clang/lib/AST/ExprConstantMeta.cpp @@ -4532,6 +4532,24 @@ bool define_aggregate(APValue &Result, ASTContext &C, MetaActions &Meta, if (!Definition) return true; + if (Definition->isUnion()) { + // union specific behaviour of "define_aggregate" + + for (CXXConstructorDecl *CD : Definition->ctors()) { + if (CD->isDefaultConstructor() && CD->isDeleted()) { + CD->setDeletedAsWritten(false); + CD->setExplicitlyDefaulted(true); + } + } + + if (CXXDestructorDecl *DD = Definition->getDestructor()) { + if (DD->isDeleted()) { + DD->setDeletedAsWritten(false); + DD->setExplicitlyDefaulted(true); + } + } + } + C.recordClassMemberSpecHash(ToComplete, MemberSpecHash); return SetAndSucceed(Result, makeReflection(ToComplete)); } diff --git a/clang/lib/Sema/SemaReflect.cpp b/clang/lib/Sema/SemaReflect.cpp index 1a30ae4776bcfd..88410c07aca0b8 100644 --- a/clang/lib/Sema/SemaReflect.cpp +++ b/clang/lib/Sema/SemaReflect.cpp @@ -515,6 +515,8 @@ class MetaActionsImpl : public MetaActions { // Iterate over member specs. unsigned AnonMemCtr = 0; + bool AtLeastOneMemberIsNonTriviallyConstr = false; + for (TagDataMemberSpec *MemberSpec : MemberSpecs) { // Build the member declaration. unsigned DiagID; @@ -574,11 +576,26 @@ class MetaActionsImpl : public MetaActions { S.Context.getSizeType(), DefinitionLoc); } + if (const CXXRecordDecl *RD = MemberSpec->Ty->getAsCXXRecordDecl()) { + if (RD->hasUserDeclaredConstructor()) { + AtLeastOneMemberIsNonTriviallyConstr = true; + } + } + VirtSpecifiers VS; S.ActOnCXXMemberDeclarator(&ClsScope, MemberAS, MemberDeclarator, MTP, BitWidthCE, VS, ICIS_NoInit); } + const bool NeedToGenerateDefaultConstr = + IncompleteDecl->getTagKind() == TagTypeKind::Union && + AtLeastOneMemberIsNonTriviallyConstr && + NewDecl->needsImplicitDefaultConstructor(); + + if (NeedToGenerateDefaultConstr) { + S.DeclareImplicitDefaultConstructor(NewDecl); + } + // Finish the member-specification and the class definition. S.ActOnFinishCXXMemberSpecification(&ClsScope, NewDecl->getBeginLoc(), NewDecl, SourceLocation{}, diff --git a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp index 8390e69df5bc26..b694f701e6a821 100644 --- a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp +++ b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp @@ -316,22 +316,39 @@ struct A { } }; +// all members of union are trivially constructible and destructible union U; static_assert(is_type(define_aggregate( ^^U, { data_member_spec(^^int), - data_member_spec(^^A), + data_member_spec(^^char), }))); static_assert(std::is_default_constructible::value == true); -static_assert(std::is_trivially_constructible::value == false); +static_assert(std::is_trivially_constructible::value == true); static_assert(std::is_destructible::value == true); -static_assert(std::is_trivially_destructible::value == false); +static_assert(std::is_trivially_destructible::value == true); + +// at least one member of union has non-trivial constructor and destructor +union UU; +static_assert(is_type(define_aggregate( + ^^UU, + { + data_member_spec(^^int), + data_member_spec(^^A), + }))); + +static_assert(std::is_default_constructible::value == true); +static_assert(std::is_trivially_constructible::value == false); + +static_assert(std::is_destructible::value == true); +static_assert(std::is_trivially_destructible::value == false); + } // namespace non_trivial_constructor_and_destructor int main() { - non_trivial_constructor_and_destructor::U u{1}; - u.~U(); + non_trivial_constructor_and_destructor::UU u; + u.~UU(); } From fddb4ca6bf53760befb3ca5715b9229bd0603298 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Sat, 23 Nov 2024 12:08:33 +0000 Subject: [PATCH 05/10] empty body instead of explicit default Signed-off-by: Valentyn Yukhymenko --- clang/lib/AST/ExprConstantMeta.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/clang/lib/AST/ExprConstantMeta.cpp b/clang/lib/AST/ExprConstantMeta.cpp index 919107d0aac215..f8d665fe53e6dc 100644 --- a/clang/lib/AST/ExprConstantMeta.cpp +++ b/clang/lib/AST/ExprConstantMeta.cpp @@ -4538,14 +4538,22 @@ bool define_aggregate(APValue &Result, ASTContext &C, MetaActions &Meta, for (CXXConstructorDecl *CD : Definition->ctors()) { if (CD->isDefaultConstructor() && CD->isDeleted()) { CD->setDeletedAsWritten(false); - CD->setExplicitlyDefaulted(true); + + auto *EmptyBody = CompoundStmt::Create( + C, {}, FPOptionsOverride(), CD->getBeginLoc(), CD->getEndLoc()); + CD->setBody(EmptyBody); + CD->setAccess(AS_public); } } if (CXXDestructorDecl *DD = Definition->getDestructor()) { if (DD->isDeleted()) { DD->setDeletedAsWritten(false); - DD->setExplicitlyDefaulted(true); + + auto *EmptyBody = CompoundStmt::Create( + C, {}, FPOptionsOverride(), DD->getBeginLoc(), DD->getEndLoc()); + DD->setBody(EmptyBody); + DD->setAccess(AS_public); } } } From 5a2a884abff9e96fa46d1d74d1e3335257f1c965 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Sat, 23 Nov 2024 12:20:16 +0000 Subject: [PATCH 06/10] small refactoring Signed-off-by: Valentyn Yukhymenko --- clang/lib/AST/ExprConstantMeta.cpp | 6 ++---- .../reflection/define-aggregate.pass.cpp | 21 +++++++++---------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/clang/lib/AST/ExprConstantMeta.cpp b/clang/lib/AST/ExprConstantMeta.cpp index f8d665fe53e6dc..3cee9b24aac545 100644 --- a/clang/lib/AST/ExprConstantMeta.cpp +++ b/clang/lib/AST/ExprConstantMeta.cpp @@ -4539,8 +4539,7 @@ bool define_aggregate(APValue &Result, ASTContext &C, MetaActions &Meta, if (CD->isDefaultConstructor() && CD->isDeleted()) { CD->setDeletedAsWritten(false); - auto *EmptyBody = CompoundStmt::Create( - C, {}, FPOptionsOverride(), CD->getBeginLoc(), CD->getEndLoc()); + auto *EmptyBody = CompoundStmt::CreateEmpty(C, 0, false); CD->setBody(EmptyBody); CD->setAccess(AS_public); } @@ -4550,8 +4549,7 @@ bool define_aggregate(APValue &Result, ASTContext &C, MetaActions &Meta, if (DD->isDeleted()) { DD->setDeletedAsWritten(false); - auto *EmptyBody = CompoundStmt::Create( - C, {}, FPOptionsOverride(), DD->getBeginLoc(), DD->getEndLoc()); + auto *EmptyBody = CompoundStmt::CreateEmpty(C, 0, false); DD->setBody(EmptyBody); DD->setAccess(AS_public); } diff --git a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp index b694f701e6a821..319856cd3dd669 100644 --- a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp +++ b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp @@ -306,15 +306,6 @@ static_assert(is_type(define_aggregate(^^S2, { namespace non_trivial_constructor_and_destructor { // https://github.com/bloomberg/clang-p2996/issues/115 -struct A { - constexpr A() { - // no-op - } - - constexpr ~A() { - // no-op - } -}; // all members of union are trivially constructible and destructible union U; @@ -332,6 +323,16 @@ static_assert(std::is_destructible::value == true); static_assert(std::is_trivially_destructible::value == true); // at least one member of union has non-trivial constructor and destructor +struct A { + constexpr A() { + // no-op + } + + constexpr ~A() { + // no-op + } +}; + union UU; static_assert(is_type(define_aggregate( ^^UU, @@ -349,6 +350,4 @@ static_assert(std::is_trivially_destructible::value == false); } // namespace non_trivial_constructor_and_destructor int main() { - non_trivial_constructor_and_destructor::UU u; - u.~UU(); } From a5e5efe28a170bbab10774d93c2c47dbf65174e9 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Sat, 23 Nov 2024 12:43:52 +0000 Subject: [PATCH 07/10] fix bug with userProvided flag + add asserts for safety Signed-off-by: Valentyn Yukhymenko --- clang/lib/AST/ExprConstantMeta.cpp | 33 ++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/clang/lib/AST/ExprConstantMeta.cpp b/clang/lib/AST/ExprConstantMeta.cpp index 3cee9b24aac545..6063d5fb5ecc37 100644 --- a/clang/lib/AST/ExprConstantMeta.cpp +++ b/clang/lib/AST/ExprConstantMeta.cpp @@ -4537,21 +4537,38 @@ bool define_aggregate(APValue &Result, ASTContext &C, MetaActions &Meta, for (CXXConstructorDecl *CD : Definition->ctors()) { if (CD->isDefaultConstructor() && CD->isDeleted()) { - CD->setDeletedAsWritten(false); + CXXConstructorDecl *CanonicalCD = CD->getCanonicalDecl(); - auto *EmptyBody = CompoundStmt::CreateEmpty(C, 0, false); - CD->setBody(EmptyBody); - CD->setAccess(AS_public); + CanonicalCD->setImplicit(false); + CanonicalCD->setDeletedAsWritten(false); + CanonicalCD->setDefaulted(false); + CanonicalCD->setAccess(AS_public); + + auto *EmptyBody = + CompoundStmt::CreateEmpty(CanonicalCD->getASTContext(), 0, false); + CanonicalCD->setBody(EmptyBody); + + assert(CD->isUserProvided()); + assert(CD->isDefaultConstructor()); + assert(!CD->isDeleted()); } } if (CXXDestructorDecl *DD = Definition->getDestructor()) { if (DD->isDeleted()) { - DD->setDeletedAsWritten(false); + CXXDestructorDecl *CanonicalDD = DD->getCanonicalDecl(); + + CanonicalDD->setImplicit(false); + CanonicalDD->setDeletedAsWritten(false); + CanonicalDD->setDefaulted(false); + CanonicalDD->setAccess(AS_public); + + auto *EmptyBody = + CompoundStmt::CreateEmpty(CanonicalDD->getASTContext(), 0, false); + CanonicalDD->setBody(EmptyBody); - auto *EmptyBody = CompoundStmt::CreateEmpty(C, 0, false); - DD->setBody(EmptyBody); - DD->setAccess(AS_public); + assert(DD->isUserProvided()); + assert(!DD->isDeleted()); } } } From 6d4b2021c22be9a3a626a35a83aec5867f0ed930 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Sat, 23 Nov 2024 12:48:09 +0000 Subject: [PATCH 08/10] small refactoring Signed-off-by: Valentyn Yukhymenko --- .../test/std/experimental/reflection/define-aggregate.pass.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp index 319856cd3dd669..17f7bbde5298bb 100644 --- a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp +++ b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp @@ -349,5 +349,4 @@ static_assert(std::is_trivially_destructible::value == false); } // namespace non_trivial_constructor_and_destructor -int main() { -} +int main() {} From bc70064e6471437e1eb04860a91b73731a35528c Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Sat, 23 Nov 2024 12:48:54 +0000 Subject: [PATCH 09/10] small refactoring Signed-off-by: Valentyn Yukhymenko --- .../test/std/experimental/reflection/define-aggregate.pass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp index 17f7bbde5298bb..a804a398a94a20 100644 --- a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp +++ b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp @@ -349,4 +349,4 @@ static_assert(std::is_trivially_destructible::value == false); } // namespace non_trivial_constructor_and_destructor -int main() {} +int main() { } From a8ab620e9c2b298ff3a1950d4132e3d98a6d4f60 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko Date: Sat, 23 Nov 2024 12:51:11 +0000 Subject: [PATCH 10/10] wording Signed-off-by: Valentyn Yukhymenko --- .../std/experimental/reflection/define-aggregate.pass.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp index a804a398a94a20..3503cc364b898d 100644 --- a/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp +++ b/libcxx/test/std/experimental/reflection/define-aggregate.pass.cpp @@ -304,7 +304,7 @@ static_assert(is_type(define_aggregate(^^S2, { } // namespace repeat_calls -namespace non_trivial_constructor_and_destructor { +namespace non_trivial_constructor_and_destructor_of_union_members { // https://github.com/bloomberg/clang-p2996/issues/115 // all members of union are trivially constructible and destructible @@ -322,7 +322,7 @@ static_assert(std::is_trivially_constructible::value == true); static_assert(std::is_destructible::value == true); static_assert(std::is_trivially_destructible::value == true); -// at least one member of union has non-trivial constructor and destructor +// at least one member of union has non-trivial constructor or destructor struct A { constexpr A() { // no-op