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

[Reflection] NFC: Workaround LLVM C++ standard library weirdness #32406

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

davezarzycki
Copy link
Contributor

Pull request #32244 introduced the use of std::stringstream but that causes vtables to be generated and we don't want that.

Pull request swiftlang#32244 introduced the use of `std::stringstream` but that
causes vtables to be generated and we don't want that.
@davezarzycki
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Contributor

@mikeash mikeash left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for the trouble!

@davezarzycki davezarzycki merged commit 9617cfe into swiftlang:master Jun 16, 2020
@davezarzycki davezarzycki deleted the pr32406 branch June 16, 2020 14:55
@davezarzycki
Copy link
Contributor Author

No problem-o. The usage of std::stringstream seems reasonable. This is surely a bug with LLVM's standard library.

@mikeash
Copy link
Contributor

mikeash commented Jun 16, 2020

These infernal machines are far too complicated.

@davezarzycki
Copy link
Contributor Author

(Hand waving about what we ought to do.) If we're going to use the C++ libraries (standard or otherwise), we ought to have a test that runs nm against the libraries and audit that all of the imported symbols are expected and reasonable.

@davezarzycki
Copy link
Contributor Author

@gottesmm
Copy link
Contributor

@davezarzycki we already do stuff like that in the integration suite. See: https://github.com/apple/swift-integration-tests/tree/master/test-snapshot-binaries. If you want to add anything, please feel free to do so!

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.

3 participants