-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[stdlib][SR-9438] Re-implement integer-to-string conversion #36636
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
Conversation
|
@swift-ci please smoke test |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@swift-ci please smoke test macOS platform |
|
@milseman Hehe, this has uncovered a bug in equality checking for
I can confirm that, with this patch [edit: without the workaround now applied; see below], I can reduce the problem to literally: print(String(42)) // "42"
print("42") // "42"
print("42" == String(42)) // falseI've figured out the problem, I think. The
All of this works perfectly when the string isn't small. Digging through the sources, I understand that a These unused bytes could also be masked before comparison, but that doesn't occur either. Since neither course of action is taken, the result is that two apparently equal strings compared as not equal. Overall, I think [Edit] |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Distinguishing between uninitialized vs 0-initialized is pretty silly for CC @atrick for the typed access stuff.
That is bad. @Catfish-Man , did you encounter this anywhere when you were testing the uninitialized init? I'll work on adding the all-bits-are-zero-until-discriminator assertion to |
Agreed about the distinction. My point here is that the zero-initialization needs to be assured by AFAICT, to fix this bug, it's just a matter of writing |
|
@swift-ci please smoke test macOS platform |
|
@swift-ci please smoke test Linux platform |
|
@swift-ci benchmark |
This comment has been minimized.
This comment has been minimized.
…and implement a hexadecimal one.
|
@swift-ci benchmark |
This comment has been minimized.
This comment has been minimized.
|
@swift-ci benchmark |
|
Build failed before running benchmark. |
|
@swift-ci benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibs
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
Small string fix is in #36667 |
|
Closing in favor of #85180 |
The referenced bug discusses several drawbacks of the status quo, which this PR attempts to address:
Int64orUInt64, base 10 or 16Stringbuffer, small when appropriate_{u}int64ToString, implementing purely in Swift(The diff is kind of garbage this time; the relevant new code is in a contiguous chunk starting here.) Let's see if this improves performance at all.
Resolves SR-9438.