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-9454: StringProtocol.lineRange(for:) returns invalid range on Linux #1841

Merged
merged 1 commit into from Jan 25, 2019

Conversation

norio-nomura
Copy link
Contributor

Fixes SR-9454

@spevans
Copy link
Collaborator

spevans commented Jan 23, 2019

@swift-ci test

@norio-nomura
Copy link
Contributor Author

We may also need to examine other places that use String.Index.init(encodedOffset:). 🤔

@norio-nomura
Copy link
Contributor Author

norio-nomura commented Jan 25, 2019

I filed a separate issue about encodedOffset: https://bugs.swift.org/browse/SR-9749

@milseman milseman requested a review from spevans January 25, 2019 00:55
@milseman
Copy link
Contributor

@spevans this change looks good to me and is an important fix for Swift 5. But, I'm not familiar with corelibs-foundation, what do you think?

@spevans
Copy link
Collaborator

spevans commented Jan 25, 2019

I think this needs an extra test on a SubString to check that the _substringOffset can be removed as the comment for it is:

  // passing _ns to the Foundation APIs. Will be 0 if self is String.

@norio-nomura
Copy link
Contributor Author

I believe that this PR will be replaced by the forthcoming PR for swift-corelibs-foundation paired with apple/swift#22108.

Copy link
Collaborator

@spevans spevans 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 fine but if it is to be superseded by a new PR it would be good to keep the test case.

@milseman
Copy link
Contributor

That PR gives new API that you can use instead of having to advance the index yourself. Basically, it will let you simplify your one line of code, but it could be held up for a while awaiting core-team feedback and maybe even some SE work. This change is good and important no matter what happens with that PR, are you ok to merge it @spevans?

@spevans
Copy link
Collaborator

spevans commented Jan 25, 2019

Yup, thanks for the explanation, lets get this fix in.

@spevans
Copy link
Collaborator

spevans commented Jan 25, 2019

@swift-ci test and merge

@swift-ci swift-ci merged commit 3fcfcd6 into apple:master Jan 25, 2019
@norio-nomura norio-nomura deleted the fix-sr-9454 branch January 25, 2019 23:06
@norio-nomura
Copy link
Contributor Author

Thanks!

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