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

SR-6032 fix private class printing with String(describing:) #12298

Merged
merged 1 commit into from Oct 9, 2017

Conversation

JGiola
Copy link
Contributor

@JGiola JGiola commented Oct 5, 2017

Private classes doesn't add extra stuff when passed to String(describing:)

I'm not at all sure if the test done in this way is correct, launched with:

./llvm/utils/lit/lit.py -sv ./build/Ninja-DebugAssert/swift-macosx-x86_64/test-macosx-x86_64/stdlib/StringDescribing.swift

will end with a success, but I haven't found how to launch all the tests for sodlib without launching all 3000.
Resolves SR-6032.

@JGiola JGiola changed the title SR-6032 SR-6032 fix private class printing with String(describing:) Oct 5, 2017
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

This looks good to me! Can you also add a negative test for String(reflecting:)? The names that get output by that should be stable enough to encode in this test case.


var StringDescribingTestSuite = TestSuite("StringDescribing")

StringDescribingTestSuite.test("String(reflecting:) shouldn't include extra stuff if the class is private") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should say "String(describing:)".

@jrose-apple
Copy link
Contributor

You can run a limited set of tests using lit's --filter option. Pass the address of the top-level test directory and then filter on "stdlib/".

@JGiola
Copy link
Contributor Author

JGiola commented Oct 6, 2017

Ok I think I got it, the new tests looks good?


StringDescribingTestSuite.test("String(reflecting:) should include extra stuff if the class is private") {
expectEqual(String(reflecting: Bar.self), "main.Bar")
expectNotEqual(String(reflecting: Foo.self), "main.Foo")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, I'd like you to put the actual thing it uses here. It's not that we care about it directly, but that we'd want to know if any part of it disappeared. (That is, "Foo" and "main.(private).Foo" would also pass this test.)

@JGiola
Copy link
Contributor Author

JGiola commented Oct 7, 2017

Something like this?


StringDescribingTestSuite.test("String(reflecting:) should include extra stuff if the class is private") {
expectEqual(String(reflecting: Bar.self), "main.Bar")
expectNotEqual(String(reflecting: Foo.self), "main.(private).Foo")
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant to actually use expectEqual here for the current format. You can even mention in a comment that no one's relying on the details of the full demangled string, but that we want to know when it changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I had not understood the first time.
But the se mangled string will be equal between different platform?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should be. The "private discriminator" part of the mangled name needs to be consistent on one platform so that rebuilding doesn't give you something different, but at the moment it's also consistent across platforms, and we have other tests that already rely on that.

Private classes doesn't add extra stuff when passed to String(describing:)
@JGiola
Copy link
Contributor Author

JGiola commented Oct 9, 2017

Ok, I think this time is the right one 😀

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Yep, let's go with that. Thanks, Jacopo!

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@jrose-apple jrose-apple self-assigned this Oct 9, 2017
@swift-ci
Copy link
Collaborator

swift-ci commented Oct 9, 2017

Build failed
Swift Test Linux Platform
Git Sha - a39dc4eaaefa0c39d746cd5b1230b47a3410b349

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 9, 2017

Build failed
Swift Test OS X Platform
Git Sha - a39dc4eaaefa0c39d746cd5b1230b47a3410b349

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

4 participants