Skip to content

Endian fix for Strings in s390x #4194

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

Merged
merged 1 commit into from
Aug 12, 2016

Conversation

garyliu1
Copy link

What's in this pull request?

This PR contains two big endian related fix in String.

  1. The change in StaticString's withUTF8Buffer is to store the UTF-8 code unit properly in the buffer.
  2. The change in StringCore's _nthContiguous is to represent the UTF-16 data correctly when it contains the high byte.

We ran validation tests and foundation tests with the change on x86-64 and s390x. No new regression is observed.

Resolved bug number: (SR-)


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library only for macOS. Simulator standard libraries and
    device standard libraries are not built.
  3. lldb is not built.
  4. The test and validation-test targets are run only for macOS. The optimized
    version of these tests are not run.

A smoke test on Linux does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library incrementally.
  3. lldb is built incrementally.
  4. The swift test and validation-test targets are run. The optimized version of these
    tests are not run.
  5. lldb is tested.

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@jrose-apple
Copy link
Contributor

Are these changes caught by existing tests, or is there a new test that should be added so that this would fail on big-endian systems without this change?

@garyliu1
Copy link
Author

garyliu1 commented Aug 10, 2016

@jrose-apple, thanks for the comment. There are existing tests catching these issues in s390x which I used for debugging.

1_stdlib/StaticSting.swift is catching UTF-8 one and 1_stdlib/Character.swift is catching the UTF-16 issue

Current test coverage would be sufficient.

@garyliu1
Copy link
Author

@swift-ci Please smoke test OS X platform

@CodaFi
Copy link
Contributor

CodaFi commented Aug 12, 2016

@garyliu1 Unfortunately, that won't work without privileges!

@swift-ci please smoke test OS X platform.

@garyliu1
Copy link
Author

@CodaFi. Thank you for getting smoke test running

@jrose-apple jrose-apple merged commit dd13eb9 into swiftlang:master Aug 12, 2016
@jrose-apple
Copy link
Contributor

Thanks, Gary! (and Robert). Given @vivkong's comment on the other s390x patch, it probably makes sense to pull this into swift-3.0-branch (following the release process). Mind making a pull request for that?

@garyliu1
Copy link
Author

@jrose-apple Thanks. We will follow up with another PR to swift-3.0-branch.

vivkong pushed a commit to linux-on-ibm-z/swift that referenced this pull request Aug 15, 2016
1. The change in StaticString's withUTF8Buffer is to store the UTF-8 code unit properly in the buffer.

2. The change in StringCore's _nthContiguous is to represent the UTF-16 data correctly when it contains the high byte.

We ran validation tests and foundation tests with the change on x86-64 and s390x. No new regression is observed.
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