-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[stdlib] Eliminate (Closed)CountableRange using conditional conformance #13342
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
6facd9a
to
05f4ac8
Compare
stdlib/public/core/Range.swift.gyb
Outdated
_precondition(!other.isEmpty, "Can't form an empty closed range") | ||
let upperBound = other.upperBound.advanced(by: -1) | ||
% else: | ||
let upperBound = other.upperBound |
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.
Don't we still need this logic for the same-type initialisers (i.e Range
's init(_: Range)
& ClosedRange
's init(_: ClosedRange)
)? Otherwise we'd be decrementing the upper bound in those cases. Or is the plan to get rid of the same-type initialisers?
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.
Yeah, this looks like a meta programming fail on my part. I'm going to de-gyb the whole thing in the next push.
There's no reason to do a Range.init
from a Range
, in fact. It should just be a deprecation warning (for people who were using it to convert the now-typealiased CountableRange
into a Range
).
b7cc166
to
02bf8a1
Compare
4109687
to
a6dc094
Compare
80aa4e9
to
3801e3f
Compare
Build comment file:Optimized (O)Regression (148)
Improvement (31)
No Changes (151)
Unoptimized (Onone)Regression (69)
Improvement (59)
No Changes (202)
Hardware Overview
|
Yikes. OK maybe not so much with the performance... Let's see if unspecializing the iterators helps. |
@swift-ci please smoke benchmark |
@swift-ci please smoke test compiler performance |
Build comment file:Optimized (O)Regression (77)
Improvement (25)
No Changes (228)
Unoptimized (Onone)Regression (23)
Improvement (22)
No Changes (285)
Hardware Overview
|
935b555
to
4b1c476
Compare
@swift-ci please smoke benchmark |
Build comment file:Optimized (O)Regression (17)
Improvement (30)
No Changes (283)
Unoptimized (Onone)Regression (19)
Improvement (30)
No Changes (281)
Hardware Overview
|
OK that's better... |
b6bcb3c
to
5dec78c
Compare
47a309a
to
d7f73fc
Compare
@swift-ci please test macOS platform |
Test with swiftlang/swift-corelibs-foundation#1410 @swift-ci please test linux platform |
🎉🎉🎉🎉 |
@natecook1000 quite |
Very cool! |
Looks like there’s a failing OS X test:
|
Ack, I didn't think about this having impact on LLDB. I'll fix that. Thanks, @xwu |
ok, looks like it's only a hard-coded test expecting a specific test that needs fixing. |
Based on groundwork by @natecook1000
Not yet ready for commit until we figure out the source compatibility/migration story.
Tests are mostly fixed up, except for things like hard-coded mangled name tests.
Range
andClosedRange
have custom iterator types now to work around some issues that seem to be related to conditional conformance, but might also bring some perf wins.