Skip to content

[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

Merged

Conversation

jrose-apple
Copy link
Contributor

@jrose-apple jrose-apple commented Feb 11, 2017

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

@slavapestov
Copy link
Contributor

This looks like it needs a rebase? Excellent work though :)

@jrose-apple
Copy link
Contributor Author

Yeah, it's "just" the module format version but I'm taking the opportunity to retest locally. Thanks!

@jrose-apple jrose-apple force-pushed the these-and-only-these-extensions branch from aabe0de to 087e2ff Compare February 11, 2017 02:09
@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - aabe0ded355e70964241a28819b00dc2af35c144
Test requested by - @jrose-apple

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - aabe0ded355e70964241a28819b00dc2af35c144
Test requested by - @jrose-apple

Copy link
Member

@DougGregor DougGregor left a 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>;
Copy link
Member

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
@jrose-apple jrose-apple force-pushed the these-and-only-these-extensions branch from 087e2ff to 9d82c1e Compare February 13, 2017 18:48
@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test

@jrose-apple jrose-apple merged commit c86f8e7 into swiftlang:master Feb 13, 2017
@jrose-apple jrose-apple deleted the these-and-only-these-extensions branch February 13, 2017 20:42
jrose-apple added a commit to jrose-apple/swift that referenced this pull request Feb 13, 2017
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants