Skip to content
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] Handle XREFs to private types #14352

Merged
merged 1 commit into from Feb 8, 2018

Conversation

jrose-apple
Copy link
Contributor

@jrose-apple jrose-apple commented Feb 2, 2018

We can encounter these when the compiler modifies an inlinable function to break apart a struct and the struct uses a private type for one of its fields. It's questionable whether we should handle this, but meanwhile this is a non-intrusive fix that preserves the performance of non-resilient libraries.

(That is, it appears this worked in Swift 4.0, though perhaps not all of the same optimizations kicked in.)

SR-6874 / rdar://problem/37072260

@jrose-apple
Copy link
Contributor Author

@gottesmm, @atrick, in case you're curious. This level of "fix" isn't that intrusive, but…

@slavapestov is going to hate this.

@swift-ci Please test

@jrose-apple
Copy link
Contributor Author

@DougGregor review ping (since it is a 4.1 regression of sorts)

@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test

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 looks sound, and I don't see any better way to do this... yeah... LGTM

XRefTypePathPieceLayout::readRecord(scratch, IID, privateDiscriminator,
onlyInNominal);
if (privateDiscriminator)
goto giveUpFastPath;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boo :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I could have written a do-while-false loop, but I think that would have been less self-documenting.

We can encounter these when the compiler modifies an inlinable
function to break apart a struct and the struct uses a private
type for one of its fields. It's questionable whether we /should/
handle this, but meanwhile this /is/ a non-intrusive fix that
preserves the performance of non-resilient libraries.

(That is, it appears this worked in Swift 4.0, though perhaps
not all of the same optimizations kicked in.)

https://bugs.swift.org/browse/SR-6874
@jrose-apple
Copy link
Contributor Author

@swift-ci Please smoke test

@jrose-apple jrose-apple merged commit af67204 into apple:master Feb 8, 2018
@jrose-apple jrose-apple deleted the xrefprehensible branch February 8, 2018 00:47
jrose-apple added a commit to jrose-apple/swift that referenced this pull request Feb 8, 2018
We can encounter these when the compiler modifies an inlinable
function to break apart a struct and the struct uses a private
type for one of its fields. It's questionable whether we /should/
handle this, but meanwhile this /is/ a non-intrusive fix that
preserves the performance of non-resilient libraries.

(That is, it appears this worked in Swift 4.0, though perhaps
not all of the same optimizations kicked in.)

https://bugs.swift.org/browse/SR-6874
(cherry picked from commit af67204)
jrose-apple added a commit to jrose-apple/swift that referenced this pull request Feb 8, 2018
We can encounter these when the compiler modifies an inlinable
function to break apart a struct and the struct uses a private
type for one of its fields. It's questionable whether we /should/
handle this, but meanwhile this /is/ a non-intrusive fix that
preserves the performance of non-resilient libraries.

(That is, it appears this worked in Swift 4.0, though perhaps
not all of the same optimizations kicked in.)

https://bugs.swift.org/browse/SR-6874
(cherry picked from commit af67204)
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.

None yet

2 participants