Skip to content

Commit 0287d65

Browse files
susmonteirosusmonteiro
authored
Merge pull request #85331 from susmonteiro/susmonteiro/revert-implicit-constructors
[cxx-interop] Revert changes to copy/move constructors definition
2 parents 1e446b4 + 2c9bb7a commit 0287d65

File tree

11 files changed

+75
-227
lines changed

11 files changed

+75
-227
lines changed

‎include/swift/ClangImporter/ClangImporter.h‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -733,12 +733,6 @@ ValueDecl *getImportedMemberOperator(const DeclBaseName &name,
733733
/// as permissive as the input C++ access.
734734
AccessLevel convertClangAccess(clang::AccessSpecifier access);
735735

736-
/// Lookup and return the copy constructor of \a decl
737-
///
738-
/// Returns nullptr if \a decl doesn't have a valid copy constructor
739-
const clang::CXXConstructorDecl *
740-
findCopyConstructor(const clang::CXXRecordDecl *decl);
741-
742736
/// Read file IDs from 'private_fileid' Swift attributes on a Clang decl.
743737
///
744738
/// May return >1 fileID when a decl is annotated more than once, which should

‎lib/ClangImporter/ClangImporter.cpp‎

Lines changed: 11 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -8322,39 +8322,16 @@ bool importer::isViewType(const clang::CXXRecordDecl *decl) {
83228322
return !hasOwnedValueAttr(decl) && hasPointerInSubobjects(decl);
83238323
}
83248324

8325-
const clang::CXXConstructorDecl *
8326-
importer::findCopyConstructor(const clang::CXXRecordDecl *decl) {
8327-
for (auto ctor : decl->ctors()) {
8328-
if (ctor->isCopyConstructor() &&
8329-
// FIXME: Support default arguments (rdar://142414553)
8330-
ctor->getNumParams() == 1 && ctor->getAccess() == clang::AS_public &&
8331-
!ctor->isDeleted() && !ctor->isIneligibleOrNotSelected())
8332-
return ctor;
8333-
}
8334-
return nullptr;
8335-
}
8336-
8337-
static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl,
8338-
ClangImporter::Implementation *importerImpl) {
8339-
if (!decl->hasSimpleCopyConstructor()) {
8340-
auto *copyCtor = findCopyConstructor(decl);
8341-
if (!copyCtor)
8342-
return false;
8343-
8344-
if (!copyCtor->isDefaulted())
8345-
return true;
8346-
}
8325+
static bool hasCopyTypeOperations(const clang::CXXRecordDecl *decl) {
8326+
if (decl->hasSimpleCopyConstructor())
8327+
return true;
83478328

8348-
// If the copy constructor is defaulted or implicitly declared, we should only
8349-
// import the type as copyable if all its fields are also copyable
8350-
// FIXME: we should also look at the bases
8351-
return llvm::none_of(decl->fields(), [&](clang::FieldDecl *field) {
8352-
if (const auto *rd = field->getType()->getAsRecordDecl()) {
8353-
return (!field->getType()->isReferenceType() &&
8354-
!field->getType()->isPointerType() &&
8355-
recordHasMoveOnlySemantics(rd, importerImpl));
8356-
}
8357-
return false;
8329+
return llvm::any_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
8330+
return ctor->isCopyConstructor() && !ctor->isDeleted() &&
8331+
!ctor->isIneligibleOrNotSelected() &&
8332+
// FIXME: Support default arguments (rdar://142414553)
8333+
ctor->getNumParams() == 1 &&
8334+
ctor->getAccess() == clang::AccessSpecifier::AS_public;
83588335
});
83598336
}
83608337

@@ -8553,8 +8530,8 @@ CxxValueSemantics::evaluate(Evaluator &evaluator,
85538530
return CxxValueSemanticsKind::Copyable;
85548531
}
85558532

8556-
bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) &&
8557-
hasCopyTypeOperations(cxxRecordDecl, importerImpl);
8533+
bool isCopyable = !hasNonCopyableAttr(cxxRecordDecl) &&
8534+
hasCopyTypeOperations(cxxRecordDecl);
85588535
bool isMovable = hasMoveTypeOperations(cxxRecordDecl);
85598536

85608537
if (!hasDestroyTypeOperations(cxxRecordDecl) || (!isCopyable && !isMovable)) {

‎lib/ClangImporter/ImportDecl.cpp‎

Lines changed: 32 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -186,15 +186,6 @@ bool importer::recordHasReferenceSemantics(
186186
return semanticsKind == CxxRecordSemanticsKind::Reference;
187187
}
188188

189-
bool importer::recordHasMoveOnlySemantics(
190-
const clang::RecordDecl *decl,
191-
ClangImporter::Implementation *importerImpl) {
192-
auto semanticsKind = evaluateOrDefault(
193-
importerImpl->SwiftContext.evaluator,
194-
CxxValueSemantics({decl->getTypeForDecl(), importerImpl}), {});
195-
return semanticsKind == CxxValueSemanticsKind::MoveOnly;
196-
}
197-
198189
bool importer::hasImmortalAttrs(const clang::RecordDecl *decl) {
199190
return decl->hasAttrs() && llvm::any_of(decl->getAttrs(), [](auto *attr) {
200191
if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr))
@@ -2104,7 +2095,10 @@ namespace {
21042095
}
21052096

21062097
bool recordHasMoveOnlySemantics(const clang::RecordDecl *decl) {
2107-
return importer::recordHasMoveOnlySemantics(decl, &Impl);
2098+
auto semanticsKind = evaluateOrDefault(
2099+
Impl.SwiftContext.evaluator,
2100+
CxxValueSemantics({decl->getTypeForDecl(), &Impl}), {});
2101+
return semanticsKind == CxxValueSemanticsKind::MoveOnly;
21082102
}
21092103

21102104
void markReturnsUnsafeNonescapable(AbstractFunctionDecl *fd) {
@@ -2283,9 +2277,13 @@ namespace {
22832277
loc, ArrayRef<InheritedEntry>(), nullptr, dc);
22842278
Impl.ImportedDecls[{decl->getCanonicalDecl(), getVersion()}] = result;
22852279

2286-
if (decl->isInStdNamespace() && decl->getName() == "promise" && recordHasMoveOnlySemantics(decl)) {
2287-
// Do not import std::promise.
2288-
return nullptr;
2280+
if (recordHasMoveOnlySemantics(decl)) {
2281+
if (decl->isInStdNamespace() && decl->getName() == "promise") {
2282+
// Do not import std::promise.
2283+
return nullptr;
2284+
}
2285+
result->addAttribute(new (Impl.SwiftContext)
2286+
MoveOnlyAttr(/*Implicit=*/true));
22892287
}
22902288

22912289
// FIXME: Figure out what to do with superclasses in C++. One possible
@@ -2532,11 +2530,6 @@ namespace {
25322530
cxxRecordDecl->ctors().empty());
25332531
}
25342532

2535-
if (recordHasMoveOnlySemantics(decl)) {
2536-
result->getAttrs().add(new (Impl.SwiftContext)
2537-
MoveOnlyAttr(/*Implicit=*/true));
2538-
}
2539-
25402533
// TODO: builtin "zeroInitializer" does not work with non-escapable
25412534
// types yet. Don't generate an initializer.
25422535
if (hasZeroInitializableStorage && needsEmptyInitializer &&
@@ -3132,13 +3125,15 @@ namespace {
31323125
// instantiate its copy constructor.
31333126
bool isExplicitlyNonCopyable = hasNonCopyableAttr(decl);
31343127

3128+
clang::CXXConstructorDecl *copyCtor = nullptr;
3129+
clang::CXXConstructorDecl *moveCtor = nullptr;
31353130
clang::CXXConstructorDecl *defaultCtor = nullptr;
31363131
if (decl->needsImplicitCopyConstructor() && !isExplicitlyNonCopyable) {
3137-
clangSema.DeclareImplicitCopyConstructor(
3132+
copyCtor = clangSema.DeclareImplicitCopyConstructor(
31383133
const_cast<clang::CXXRecordDecl *>(decl));
31393134
}
31403135
if (decl->needsImplicitMoveConstructor()) {
3141-
clangSema.DeclareImplicitMoveConstructor(
3136+
moveCtor = clangSema.DeclareImplicitMoveConstructor(
31423137
const_cast<clang::CXXRecordDecl *>(decl));
31433138
}
31443139
if (decl->needsImplicitDefaultConstructor()) {
@@ -3155,13 +3150,28 @@ namespace {
31553150
// Note: we use "doesThisDeclarationHaveABody" here because
31563151
// that's what "DefineImplicitCopyConstructor" checks.
31573152
!declCtor->doesThisDeclarationHaveABody()) {
3158-
if (declCtor->isDefaultConstructor()) {
3153+
if (declCtor->isCopyConstructor()) {
3154+
if (!copyCtor && !isExplicitlyNonCopyable)
3155+
copyCtor = declCtor;
3156+
} else if (declCtor->isMoveConstructor()) {
3157+
if (!moveCtor)
3158+
moveCtor = declCtor;
3159+
} else if (declCtor->isDefaultConstructor()) {
31593160
if (!defaultCtor)
31603161
defaultCtor = declCtor;
31613162
}
31623163
}
31633164
}
31643165
}
3166+
if (copyCtor && !isExplicitlyNonCopyable &&
3167+
!decl->isAnonymousStructOrUnion()) {
3168+
clangSema.DefineImplicitCopyConstructor(clang::SourceLocation(),
3169+
copyCtor);
3170+
}
3171+
if (moveCtor && !decl->isAnonymousStructOrUnion()) {
3172+
clangSema.DefineImplicitMoveConstructor(clang::SourceLocation(),
3173+
moveCtor);
3174+
}
31653175
if (defaultCtor) {
31663176
clangSema.DefineImplicitDefaultConstructor(clang::SourceLocation(),
31673177
defaultCtor);
@@ -3170,8 +3180,7 @@ namespace {
31703180
if (decl->needsImplicitDestructor()) {
31713181
auto dtor = clangSema.DeclareImplicitDestructor(
31723182
const_cast<clang::CXXRecordDecl *>(decl));
3173-
if (!dtor->isDeleted() && !dtor->isIneligibleOrNotSelected())
3174-
clangSema.DefineImplicitDestructor(clang::SourceLocation(), dtor);
3183+
clangSema.DefineImplicitDestructor(clang::SourceLocation(), dtor);
31753184
}
31763185
}
31773186

‎lib/ClangImporter/ImporterImpl.h‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1974,11 +1974,6 @@ namespace importer {
19741974
bool recordHasReferenceSemantics(const clang::RecordDecl *decl,
19751975
ClangImporter::Implementation *importerImpl);
19761976

1977-
/// Returns true if the given C/C++ record should be imported as non-copyable into
1978-
/// Swift.
1979-
bool recordHasMoveOnlySemantics(const clang::RecordDecl *decl,
1980-
ClangImporter::Implementation *importerImpl);
1981-
19821977
/// Whether this is a forward declaration of a type. We ignore forward
19831978
/// declarations in certain cases, and instead process the real declarations.
19841979
bool isForwardDeclOfType(const clang::Decl *decl);

‎lib/IRGen/GenStruct.cpp‎

Lines changed: 29 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,15 @@ namespace {
553553
const auto *cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl);
554554
if (!cxxRecordDecl)
555555
return nullptr;
556-
return importer::findCopyConstructor(cxxRecordDecl);
556+
for (auto ctor : cxxRecordDecl->ctors()) {
557+
if (ctor->isCopyConstructor() &&
558+
// FIXME: Support default arguments (rdar://142414553)
559+
ctor->getNumParams() == 1 &&
560+
ctor->getAccess() == clang::AS_public && !ctor->isDeleted() &&
561+
!ctor->isIneligibleOrNotSelected())
562+
return ctor;
563+
}
564+
return nullptr;
557565
}
558566

559567
const clang::CXXConstructorDecl *findMoveConstructor() const {
@@ -611,7 +619,7 @@ namespace {
611619
/*invocation subs*/ SubstitutionMap(), IGF.IGM.Context);
612620
}
613621

614-
void emitCopyWithCopyOrMoveConstructor(
622+
void emitCopyWithCopyConstructor(
615623
IRGenFunction &IGF, SILType T,
616624
const clang::CXXConstructorDecl *copyConstructor, llvm::Value *src,
617625
llvm::Value *dest) const {
@@ -621,27 +629,7 @@ namespace {
621629

622630
auto &ctx = IGF.IGM.Context;
623631
auto *importer = static_cast<ClangImporter *>(ctx.getClangModuleLoader());
624-
625-
if (copyConstructor->isDefaulted() &&
626-
copyConstructor->getAccess() == clang::AS_public &&
627-
!copyConstructor->isDeleted() &&
628-
!copyConstructor->isIneligibleOrNotSelected() &&
629-
// Note: we use "doesThisDeclarationHaveABody" here because
630-
// that's what "DefineImplicitCopyConstructor" checks.
631-
!copyConstructor->doesThisDeclarationHaveABody()) {
632-
assert(!copyConstructor->getParent()->isAnonymousStructOrUnion() &&
633-
"Cannot do codegen of special member functions of anonymous "
634-
"structs/unions");
635-
if (copyConstructor->isCopyConstructor())
636-
importer->getClangSema().DefineImplicitCopyConstructor(
637-
clang::SourceLocation(),
638-
const_cast<clang::CXXConstructorDecl *>(copyConstructor));
639-
else
640-
importer->getClangSema().DefineImplicitMoveConstructor(
641-
clang::SourceLocation(),
642-
const_cast<clang::CXXConstructorDecl *>(copyConstructor));
643-
}
644-
632+
645633
auto &diagEngine = importer->getClangSema().getDiagnostics();
646634
clang::DiagnosticErrorTrap trap(diagEngine);
647635
auto clangFnAddr =
@@ -821,9 +809,9 @@ namespace {
821809
Address srcAddr, SILType T,
822810
bool isOutlined) const override {
823811
if (auto copyConstructor = findCopyConstructor()) {
824-
emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
825-
srcAddr.getAddress(),
826-
destAddr.getAddress());
812+
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
813+
srcAddr.getAddress(),
814+
destAddr.getAddress());
827815
return;
828816
}
829817
StructTypeInfoBase<AddressOnlyCXXClangRecordTypeInfo, FixedTypeInfo,
@@ -836,9 +824,9 @@ namespace {
836824
SILType T, bool isOutlined) const override {
837825
if (auto copyConstructor = findCopyConstructor()) {
838826
destroy(IGF, destAddr, T, isOutlined);
839-
emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
840-
srcAddr.getAddress(),
841-
destAddr.getAddress());
827+
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
828+
srcAddr.getAddress(),
829+
destAddr.getAddress());
842830
return;
843831
}
844832
StructTypeInfoBase<AddressOnlyCXXClangRecordTypeInfo, FixedTypeInfo,
@@ -850,15 +838,17 @@ namespace {
850838
SILType T, bool isOutlined,
851839
bool zeroizeIfSensitive) const override {
852840
if (auto moveConstructor = findMoveConstructor()) {
853-
emitCopyWithCopyOrMoveConstructor(IGF, T, moveConstructor,
854-
src.getAddress(), dest.getAddress());
841+
emitCopyWithCopyConstructor(IGF, T, moveConstructor,
842+
src.getAddress(),
843+
dest.getAddress());
855844
destroy(IGF, src, T, isOutlined);
856845
return;
857846
}
858847

859848
if (auto copyConstructor = findCopyConstructor()) {
860-
emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
861-
src.getAddress(), dest.getAddress());
849+
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
850+
src.getAddress(),
851+
dest.getAddress());
862852
destroy(IGF, src, T, isOutlined);
863853
return;
864854
}
@@ -872,16 +862,18 @@ namespace {
872862
bool isOutlined) const override {
873863
if (auto moveConstructor = findMoveConstructor()) {
874864
destroy(IGF, dest, T, isOutlined);
875-
emitCopyWithCopyOrMoveConstructor(IGF, T, moveConstructor,
876-
src.getAddress(), dest.getAddress());
865+
emitCopyWithCopyConstructor(IGF, T, moveConstructor,
866+
src.getAddress(),
867+
dest.getAddress());
877868
destroy(IGF, src, T, isOutlined);
878869
return;
879870
}
880871

881872
if (auto copyConstructor = findCopyConstructor()) {
882873
destroy(IGF, dest, T, isOutlined);
883-
emitCopyWithCopyOrMoveConstructor(IGF, T, copyConstructor,
884-
src.getAddress(), dest.getAddress());
874+
emitCopyWithCopyConstructor(IGF, T, copyConstructor,
875+
src.getAddress(),
876+
dest.getAddress());
885877
destroy(IGF, src, T, isOutlined);
886878
return;
887879
}

0 commit comments

Comments
 (0)