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

[stdlib] Don't try to construct invalid UnicodeScalars in CharacterSet #13676

Merged
merged 1 commit into from Jan 3, 2018

Conversation

Projects
None yet
3 participants
@kasei
Copy link
Contributor

kasei commented Jan 2, 2018

Inserting and removing a single Unicode.Scalar in a CharacterSet must not
attempt to create a Range<Unicode.Scalar> because the upperBound value might
not always be a valid UnicodeScalar. This patch changes insert(_:) and remove(_:)
to construct and use a ClosedRange<Unicode.Scalar> instead.

This fixes what I believe to be a regression to the fix for SR-2988.

[stdlib] Don't try to construct invalid `UnicodeScalar`s in CharacterSet
Inserting and removing a single Unicode.Scalar in a CharacterSet must not
attempt to create a Range<Unicode.Scalar> because the upperBound value might
not always be a valid UnicodeScalar.

This fixes a regression to the fix for SR-2988.

@kasei kasei changed the title [stdlib] Don't try to construct invalid `UnicodeScalar`s in CharacterSet [stdlib] Don't try to construct invalid UnicodeScalars in CharacterSet Jan 2, 2018

@jrose-apple jrose-apple requested a review from phausler Jan 2, 2018

@phausler
Copy link
Member

phausler left a comment

looks reasonable to me, can we get this over on swift-corelibs-foundation too?

@kasei

This comment has been minimized.

Copy link
Contributor Author

kasei commented Jan 2, 2018

@phausler I'm not very familiar with these repos. Should I just look into applying the same patch to the CharacterSet.swift in apple/swift-corelibs-foundation? Can you point me to where tests should go? Thanks!

@phausler

This comment has been minimized.

Copy link
Member

phausler commented Jan 2, 2018

yep, and for the unit test you can add them to TestCharacterSet.swift (it is relatively similar looking to the unit test you added).

@swift-ci please test and merge

@kasei

This comment has been minimized.

Copy link
Contributor Author

kasei commented Jan 2, 2018

@phausler the swift-corelibs-foundation code looks like it might still align with the fix from SR-2988. It's not clear to me how to directly test swift-corelibs-foundation (I had some trouble building with the Xcode projects and workspaces), but I wouldn't be surprised if this case is already covered by that code. Are you able to confirm that?

@swift-ci swift-ci merged commit cd8c8e5 into apple:master Jan 3, 2018

4 of 5 checks passed

Test and Merge Build started.
Details
Swift Test Linux Platform 10664 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 53440 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details

@kasei kasei deleted the kasei:characterset-boundary branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.