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

Fix swiftIdentifier crash with empty strings #105

Merged
merged 3 commits into from
Oct 1, 2018
Merged

Conversation

djbe
Copy link
Member

@djbe djbe commented Oct 1, 2018

@@ -114,10 +114,10 @@ enum SwiftIdentifier {
static func prefixWithUnderscoreIfNeeded(string: String,
forbiddenChars exceptions: String = "") -> String {

let (head, _) = identifierCharacterSets(exceptions: exceptions)
guard let firstChar = string.unicodeScalars.first,
!string.isEmpty else { return "" }
Copy link
Contributor

Choose a reason for hiding this comment

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

string.isEmpty can't ever be false if string.unicodeScalars.first is not nil, right? If string is empty, then string.unocodeScalars.first will return nil and already make the condition fail anyway

Copy link
Member Author

@djbe djbe Oct 1, 2018

Choose a reason for hiding this comment

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

You'd think so, wouldn't you? Give it a try, for some absolutely bizarre reason, the isEmpty check is needed 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait seriously?

isEmpty checks on characters, unicodeScalars is different, so that could make sense, maybe… but can't see an example

Do you mean that if we don't test isEmpty, the test on "" fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

It throws an exception (SIGINT or something I think)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just tested by cloning your branch and removing that !string.isEmpty in the condition and all test passed successfully, without SIGINT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which Xcode version?

Copy link
Contributor

Choose a reason for hiding this comment

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

10.0 final

@djbe djbe merged commit f823d0f into master Oct 1, 2018
@djbe djbe deleted the fix/swift-identifier branch October 1, 2018 20:13
@djbe djbe added this to the 2.7.1 milestone Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants