-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[WIP] [stdlib] Consolidate integer-to-string implementations #14401
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 |
@swift-ci Please smoke benchmark |
Build comment file:Optimized (O)Regression (3)
Improvement (11)
No Changes (346)
Unoptimized (Onone)Regression (14)
Improvement (36)
No Changes (310)
Hardware Overview
|
@swift-ci please test compiler performance |
!!! Couldn't read commit file !!! |
@swift-ci please test compiler performance |
1 similar comment
@swift-ci please test compiler performance |
Hi @graydon - if some test projects fail to build, do they get discounted from both runs, allowing us to still use these results as a guide? |
@swift-ci please test source compatibility |
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.
👍
} | ||
|
||
let hasLetters = radix > 10 | ||
let ascii: (UInt8) -> UInt8 = { digit in |
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.
Can these be proper inner func
s instead? That would help readability a little, imo.
@airspeedswift I'm afraid not, that notice is/was designed for the earlier presentation form, when we split the results by module in the comment we posted back (i.e. to tell you the reader which modules to ignore). We don't have machinery in place to automatically drop data from modules that failed; all the data aggregates into a single location and we only separate it out if asked. It's a good idea, just hasn't been done yet (but filed: rdar://37281924) |
@_versioned | ||
@_transparent | ||
internal func _description( | ||
radix: Int = 10, uppercase: Bool = false |
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 think it would be better to leave out the default values on this internal function, so we can't accidentally forget to pass on the values from the public API.
@swift-ci please smoke test compiler performance |
Not sure why those particular projects failed on the compiler performance report, but I don't think the smoke version includes them so maybe that one'll be ok... |
Build comment file:Summary for master smoketestUnexpected test results, stats may be off for 3 No regressions above thresholds Debugdebug briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
debug detailedRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
Releaserelease briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
release detailedRegressed (0)
Improved (1)
Unchanged (delta < 1.0% or delta < 100.0ms) (22)
|
Cool. Since this PR has had hours and hours of testing, I'll go ahead and merge if it's OK with all. Then, I'll address the reviewer points in a follow-up PR which shouldn't require nearly as much testing. |
ok by me! |
There's a lot of code to convert integers to strings. Let's consolidate, make it appropriate for all
BinaryInteger
types, and simplify the web of internal methods.