-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Serialization] Improve extensions of nested types with the same name #7397
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
[Serialization] Improve extensions of nested types with the same name #7397
Conversation
This looks like it needs a rebase? Excellent work though :) |
Yeah, it's "just" the module format version but I'm taking the opportunity to retest locally. Thanks! |
aabe0de
to
087e2ff
Compare
@swift-ci Please test |
Build failed |
Build failed |
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.
It does feel a bit heavy to use the mangled name here, but it's definitely a good, descriptive key.
@@ -128,12 +128,16 @@ class Serializer { | |||
|
|||
// In-memory representation of what will eventually be an on-disk | |||
// hash table of all defined Objective-C methods. | |||
using ObjCMethodTable = llvm::DenseMap<ObjCSelector, ObjCMethodTableData>; | |||
using ObjCMethodTable = llvm::MapVector<ObjCSelector, ObjCMethodTableData>; |
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.
Good drive-by
Previously looking up an extension would result in all extensions for types with the same name (nested or not) being deserialized; this could even bring in base types that had not been deserialized yet. Add in a string to distinguish an extension's base type; in the top-level case this is just a module name, but for nested types it's a full mangled name. This is a little heavier than I'd like it to be, since it means we mangle names and then throw them away, and since it means there's a whole bunch of extra string data in the module just for uniquely identifying a declaration. But it's correct, and does less work than before, and fixes a circularity issue with a nested type A.B.A that apparently used to work. https://bugs.swift.org/browse/SR-3915
087e2ff
to
9d82c1e
Compare
@swift-ci Please smoke test |
…swiftlang#7397) Previously looking up an extension would result in all extensions for types with the same name (nested or not) being deserialized; this could even bring in base types that had not been deserialized yet. Add in a string to distinguish an extension's base type; in the top-level case this is just a module name, but for nested types it's a full mangled name. This is a little heavier than I'd like it to be, since it means we mangle names and then throw them away, and since it means there's a whole bunch of extra string data in the module just for uniquely identifying a declaration. But it's correct, and does less work than before, and fixes a circularity issue with a nested type A.B.A that apparently used to work. https://bugs.swift.org/browse/SR-3915
Previously looking up an extension would result in all extensions for types with the same name (nested or not) being deserialized; this could even bring in base types that had not been deserialized yet. Add in a string to distinguish an extension's base type; in the top-level case this is just a module name, but for nested types it's a full mangled name.
This is a little heavier than I'd like it to be, since it means we mangle names and then throw them away, and since it means there's a whole bunch of extra string data in the module just for uniquely identifying a declaration. But it's correct, and does less work than before, and fixes a circularity issue with a nested type A.B.A that apparently used to work.
SR-3915