[stdlib]Un-revert string comparison #14694
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
February 16, 2018 17:50
This reverts commit abe6a6d. # Conflicts: # stdlib/public/SwiftShims/UnicodeShims.h # stdlib/public/stubs/UnicodeNormalization.cpp
|
Please test with the following pull request: @swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please smoke benchmark |
|
Please test with the following pull request: @swift-ci please test |
|
Build failed |
Build comment file:Optimized (O)Regression (9)
Improvement (40)
No Changes (329)
Unoptimized (Onone)Regression (45)
Improvement (31)
No Changes (302)
Hardware Overview |
|
Please test with the following pull request: @swift-ci please clean test Linux platform |
|
Build failed |
|
Please test with the following pull request: @swift-ci please test Linux platform |
|
Build failed |
|
Please test with the following pull request: @swift-ci please test |
|
Build failed |
|
Build failed |
|
@lancep Does this fix the sporadic failures which we saw on the bots? |
milseman
reviewed
Feb 17, 2018
| @@ -288,8 +288,8 @@ swift::__swift_stdlib_unorm2_getNFCInstance(__swift_stdlib_UErrorCode *err) { | |||
| } | |||
|
|
|||
| int32_t swift::__swift_stdlib_unorm2_normalize( | |||
| const __swift_stdlib_UNormalizer2 *norm, const __swift_stdlib_UChar *src, | |||
| __swift_int32_t len, __swift_stdlib_UChar *dst, __swift_int32_t capacity, | |||
| const __swift_stdlib_UNormalizer2 *norm, const __swift_int32_t *src, | |||
There was a problem hiding this comment.
You mean uint16_t? Why are we doing all this churn anyways?
There was a problem hiding this comment.
Good catch. I'm not sure why we started #if-ing the typedef for __swift_stdlib_UChar. Maybe the ICU headers differ on some other flavor of Linux?
…ar16_t which is supposed to be the same thing on C++11
|
@eeckstein I don't think those were related to this change now. I'm still investigating why occasionally String's capacity is higher than we expect when appending. |
|
Please test with the following pull request: @swift-ci please test |
|
Build failed |
|
Build failed |
|
Please test with the following pull request: @swift-ci please test |
|
@eeckstein This just hit that string capacity test failure again. It may be a coincidence, but it's also possible that these unrelated changes affected the test somehow. I've seen this in the past with the Array tests. It seems that on Linux, malloc occasionally will give us more space when resizing the array than it does on Darwin platforms. I'll investigate more on Monday, but I've change the test to get it passing for now, by bumping it from expecting to be <= 40 instead of 34. |
|
Build failed |
|
Please test with the following pull request: @swift-ci please test |
|
Build failed |
|
Build failed |
|
We're seeing failures from this on Linux on the 5.0 branch, since corelibs Foundation doesn't have a separate 5.0 branch at the moment. What should we do about this? cc @shahmishal, @bob-wilson, @parkera |
|
@bob-wilson Should we just have @lancep cherry-pick the change from swift:master into swift:swift-5.0-branch to unblock testing? |
|
We would need to merge @milseman's StringGuts changes there as well. |
|
@lancep, Can you cherry-pick changes to swift-5.0-branch? |
|
I don't think we want to cherry-pick all the String changes at this point. It's really just not sensible to pretend that master swift-corelibs-foundation works with 5.0 swift. |
|
I've opened swiftlang/swift-corelibs-foundation#1445 to skip the test in question |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This brings back the string comparison change along with a fix for opaque substrings.