-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Serialization] Drop overriding relationship in constructors when safe #63266
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3535,17 +3535,20 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> { | |
// its overrides after they've been compiled: if the declaration is '@objc' | ||
// and 'dynamic'. In that case, all accesses to the method or property will | ||
// go through the Objective-C method tables anyway. | ||
if (overridden->hasClangNode() || overridden->shouldUseObjCDispatch()) | ||
if (!isa<ConstructorDecl>(override) && | ||
(overridden->hasClangNode() || overridden->shouldUseObjCDispatch())) | ||
return false; | ||
|
||
// In a public-override-internal case, the override doesn't have ABI | ||
// implications. | ||
// implications. This corresponds to hiding the override keyword from the | ||
// module interface. | ||
auto isPublic = [](const ValueDecl *VD) { | ||
return VD->getFormalAccessScope(VD->getDeclContext(), | ||
/*treatUsableFromInlineAsPublic*/true) | ||
.isPublic(); | ||
}; | ||
if (isPublic(override) && !isPublic(overridden)) | ||
if (override->getDeclContext()->getParentModule()->isResilient() && | ||
isPublic(override) && !isPublic(overridden)) | ||
return false; | ||
|
||
return true; | ||
|
@@ -4499,9 +4502,12 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> { | |
uint8_t rawAccessLevel = getRawStableAccessLevel(ctor->getFormalAccess()); | ||
|
||
bool firstTimeRequired = ctor->isRequired(); | ||
if (auto *overridden = ctor->getOverriddenDecl()) | ||
auto *overridden = ctor->getOverriddenDecl(); | ||
if (overridden) { | ||
if (firstTimeRequired && overridden->isRequired()) | ||
firstTimeRequired = false; | ||
} | ||
bool overriddenAffectsABI = overriddenDeclAffectsABI(ctor, overridden); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the relationship actually useful if it doesn't? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good question. For non-resilient modules it could be used by optimizations. For resilient modules in most cases it should now be dropped by deserialization safety for the public-override-internal case, and the clang case is there more as tolerance for poor modularization and such I believe. I think we could try always dropping it when reading from a resilient module. |
||
|
||
unsigned abbrCode = S.DeclTypeAbbrCodes[ConstructorLayout::Code]; | ||
ConstructorLayout::emitRecord(S.Out, S.ScratchRecord, abbrCode, | ||
|
@@ -4517,7 +4523,8 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> { | |
ctor->getInitKind()), | ||
S.addGenericSignatureRef( | ||
ctor->getGenericSignature()), | ||
S.addDeclRef(ctor->getOverriddenDecl()), | ||
S.addDeclRef(overridden), | ||
overriddenAffectsABI, | ||
rawAccessLevel, | ||
ctor->needsNewVTableEntry(), | ||
firstTimeRequired, | ||
|
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.
Could we set
overridden = nullptr
if we're allowing compiler errors in this case (ie.MF.allowCompilerErrors
)?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.
Sure 👍