[stdlib] Adopt conditional conformance for Indices, Slice, ReversedCollection #12913
Conversation
This file contains 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
airspeedswift
changed the title
|
Wow that's a lot of deleted code. |
moiseev
reviewed
Nov 14, 2017
stdlib/public/core/Indices.swift
Outdated
| _elements: self, | ||
| startIndex: self.startIndex, | ||
| endIndex: self.endIndex) | ||
| } | ||
| } | ||
|
|
||
| % end | ||
| public typealias DefaultBidirectionalIndices<T: BidirectionalCollection> = DefaultIndices<T> |
There was a problem hiding this comment.
Deprecated in swift 4.1, perhaps?
| @@ -1,4 +1,4 @@ | |||
| //===--- RangeReplaceableCollection.swift.gyb -----------------*- swift -*-===// | |||
| //===--- RangeReplaceableCollection.swift -----------------*- swift -*-===// | |||
There was a problem hiding this comment.
Notpick: No longer 80 columns.
stdlib/public/core/Reverse.swift
Outdated
| public let _base: Base | ||
| } | ||
| @available(*, deprecated, renamed: "ReversedCollection") | ||
| typealias ReversedRandomAccessCollection<T> = ReversedCollection<T> where T: RandomAccessCollection |
stdlib/public/core/Indices.swift
Outdated
| } | ||
| } | ||
|
|
||
| extension DefaultIndices: RandomAccessCollection where Elements: RandomAccessCollection { } |
There was a problem hiding this comment.
Need to make sure we're forwarding distance(to:) and index(_:offsetBy:) to the underlying collection.
stdlib/public/core/Reverse.swift
Outdated
| public var endIndex: Index { | ||
| return ReversedRandomAccessIndex(_base.startIndex) | ||
| } | ||
| extension ReversedCollection: RandomAccessCollection where Base: RandomAccessCollection { } |
There was a problem hiding this comment.
Here too — need the index-moving methods.
airspeedswift
force-pushed
the
reversed-cond-conformance
branch
3 times, most recently
from
24b532b to
41af770
Compare
|
The first compiler crash this hits is: https://bugs.swift.org/browse/SR-6466 |
DougGregor
reviewed
Nov 29, 2017
stdlib/public/core/Reverse.swift
Outdated
| /// | ||
| /// - Complexity: O(1) | ||
| @_inlineable | ||
| public func reversed() -> LazyRandomAccessCollection< |
There was a problem hiding this comment.
Whoops, we still need this, up until the point where the Lazy collections also use conditional conformances
DougGregor
force-pushed
the
reversed-cond-conformance
branch
2 times, most recently
from
f348ce3 to
ce4d924
Compare
|
@swift-ci Please smoke test macOS platform |
|
@airspeedswift , that last commit re-breaks the test. The gyb'd conformances are redundant, so we (correctly) "redundant conformance" errors. Why did you revert my change? Because it uncovered a source-compatibility issue? |
|
@swift-ci please test source compatibility |
1 similar comment
|
@swift-ci please test source compatibility |
|
Quite curious how this fares on the compatibility suite in its current state. |
|
Note that all of the test suite failure other that the |
|
... and the crash appears to be in the |
|
The Plank and R failures are the SwiftPM failure under discussion in swiftlang/swift-package-manager#1393, so this isn't introducing any source-compatibility regressions. |
|
Turns out that we don't need that big pile of explicit typealiases in the standard library now that I've reinstated the associated type redeclarations in the |
|
@DougGregor sorry, I thought that was reverting a change I made from before you restricted the feature to only stdlib use (looks like the test was failing due to using the restricted feature). Yeah, if that original code doesn't compile, it does suggest a source compatibility issue we'll need to work around. |
|
@swift-ci Please smoke test macOS platform |
|
The source compatibility issue here is that the code in question introduced conformances for each of the protocol P { }
extension ReversedCollection : P { }
extension ReversedRandomAccessCollection : P { }
extension ReversedRangeReplaceableCollection : P { }With this PR, those conformances are now redundant, because it's stating |
|
@swift-ci please smoke test compiler performance |
DougGregor
force-pushed
the
reversed-cond-conformance
branch
from
3b9c471 to
a608f78
Compare
|
@swift-ci Please test macOS platform |
|
@swift-ci please smoke test compiler performance |
|
@swift-ci Please smoke benchmark |
|
Build failed |
|
Fixed up the |
Build comment file:Optimized (O)Regression (14)
Improvement (12)
No Changes (308)
Unoptimized (Onone)Regression (38)
Improvement (12)
No Changes (284)
Hardware Overview |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
airspeedswift
changed the title
Refactor into base definitions + extensions. Redundant associated types are still required to word around bugs in associated type inference.
These fixes are for diagnostics that changed in benign ways or for places in the testsuite that were taking advantage of standard library internals.
|
The only significant casualty on the benchmarks seems to be CharIteration in Debug. @milseman possibly something to be aware of... |
airspeedswift
force-pushed
the
reversed-cond-conformance
branch
from
9ea472e to
887fece
Compare
|
@swift-ci please test source compatibility |
|
@swift-ci please test compiler performance |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@airspeedswift merge it! merge it! :) |
Build comment file:Summary for master fullUnexpected test results, stats may be off for Kronos, vapor, Dollar, Plank, RxDataSources Regressions found (see below) Debugdebug briefRegressed (1)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (1)
debug detailedRegressed (1)
Improved (7)
Unchanged (delta < 1.0% or delta < 100.0ms) (15)
Debug-optdebug-opt briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
debug-opt detailedRegressed (1)
Improved (7)
Unchanged (delta < 1.0% or delta < 100.0ms) (15)
Wmo-ononewmo-onone briefNone wmo-onone detailedNone Releaserelease briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
release detailedRegressed (2)
Improved (7)
Unchanged (delta < 1.0% or delta < 100.0ms) (14)
|
|
Did we figure out a plan for #12913 (comment) ? If that's part of our compatibility suite it'll surely be elsewhere in the wild—I could definitely see people giving conformance to a new protocol to multiple |
|
@natecook1000, that source-compatibility issue was in our own standard library test suite. We haven't seen any such code in the wild yet. The core team discussed that specific source compatibility concern and decided that the benefits of this change vastly outweigh the cost of such breakage; if we see more widespread breakage, we'll consider additional mitigations in the compiler (e.g, ignoring redundant conformances in such cases) |
|
Thanks @DougGregor, glad to know it's been discussed! |
airspeedswift
added a commit
to airspeedswift/swift-corelibs-foundation
that referenced
this pull request
Jan 26, 2018
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 PR refactors the variations of
Slice,DefaultIndices, andReversedCollectionto be conditional conformances on each base type e.g.becomes: