-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[stdlib] Make String.makeContiguousUTF8() strictly true #82851
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] Make String.makeContiguousUTF8() strictly true #82851
Conversation
0999641
to
268eb09
Compare
@swift-ci please test linux platform |
268eb09
to
d0154c2
Compare
@swift-ci please test linux platform |
d0154c2
to
6c9b0b0
Compare
@swift-ci please test linux platform |
@swift-ci please smoke test linux platform |
1 similar comment
@swift-ci please smoke test linux platform |
bf4ada7
to
3dfdd87
Compare
@swift-ci please test |
@swift-ci please test |
@@ -312,9 +312,19 @@ extension String { | |||
@inline(never) // slow-path | |||
internal static func _copying(_ str: Substring) -> String { | |||
if _fastPath(str._wholeGuts.isFastUTF8) { | |||
return unsafe str._wholeGuts.withFastUTF8(range: str._offsetRange) { | |||
var new = unsafe str._wholeGuts.withFastUTF8(range: str._offsetRange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In non-watchOS or 64-bit environments, will the compiler not complain about this var
being unmutated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice it complaining when building on macOS. Apparently it knows to consider statements that are conditionally skipped: https://swift.godbolt.org/z/aT1naWfce
/// Contiguous strings always operate in O(1) time for withUTF8 and always | ||
/// give a result for String.UTF8View.withContiguousStorageIfAvailable. | ||
/// Contiguous strings always operate in O(1) time for withUTF8, always give | ||
/// a result for String.UTF8View.withContiguousStorageIfAvailable, and always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this comment actually inaccurate before? Did String.UTF8View.withContiguousStorageIfAvailable
actually NOT provide a result for these exceptional small strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not lying: UTF8View.withContiguousStorageIfAvailable
will rearrange the bytes of a small string in order to present contiguous storage to the closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. With this returning false
now for these strings, some might find it surprising that the former conditions are still true. However, seems like we're within our rights to do so, as this is implication ("isContiguousUTF8 -> the above guarantees hold") and not equivalence ("isContiguousUTF8 <-> the above guarantees hold").
/// Contiguous strings always operate in O(1) time for withUTF8 and always | ||
/// give a result for String.UTF8View.withContiguousStorageIfAvailable. | ||
/// Contiguous strings always operate in O(1) time for withUTF8, always give | ||
/// a result for String.UTF8View.withContiguousStorageIfAvailable, and always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. With this returning false
now for these strings, some might find it surprising that the former conditions are still true. However, seems like we're within our rights to do so, as this is implication ("isContiguousUTF8 -> the above guarantees hold") and not equivalence ("isContiguousUTF8 <-> the above guarantees hold").
@swift-ci please test linux platform |
Until now,
StringProtocol.makeContiguousUTF8()
only considered whether or not aString
orSubstring
heap allocation contained contiguous UTF8, disregarding whether a stack-allocated instance was discontiguous. This is no longer tenable when these types vendUTF8Span
andSpan
properties, so we now also consider whether the small string form contains contiguous code units. If the small form contains discontiguously-stored code units, then it is replaced by a new heap-allocated String in contiguous storage. This new code path is specific to ABI-stable, 32-bit platforms.Addresses rdar://154331399