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
New indexing model: update FlattenCollections #1670
New indexing model: update FlattenCollections #1670
Conversation
Please add a |
// are `nil`, since `_inner` is `nil` iff `_outer == base.endIndex`. | ||
_precondition(lhs._inner == nil && rhs._inner == nil) | ||
|
||
return 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.
We definitely need new tests for this code.
Since you are rewriting this code anyway, would you mind doing a few clean-ups across the file? Internal initializers like |
I think we can also add a performance optimization by providing a custom |
fatalError("FIXME: swift-3-indexing-model implement") | ||
// FIXME: swift-3-indexing-model: range checks? | ||
let nextInner = _base[i._outer].next(i._inner!) | ||
if _fastPath(nextInner != _base[i._outer].endIndex) { |
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.
You can cache base[i._outer]
in a local variable.
Yes, we should have custom implementations, because our base collections could be random access. Imagine we have a flatten collection of array of arrays. The default implementation of But I agree this is not that important. Please leave a |
That should be everything, thanks for the feedback! I'll circle back to add some tests soon. |
@@ -313,40 +270,50 @@ public struct ${Collection}< | |||
/// reachable from `startIndex` by zero or more applications of | |||
/// `successor()`. | |||
public var endIndex: Index { | |||
return ${Index}(_endIndexOfFlattened: _base) | |||
return ${Index}(_base.endIndex, nil) | |||
} |
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.
@gribozavr Should this just be a constant?
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 could be an advantage to define it as a constant -- what kind of representation do you have in mind?
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 was thinking public let endIndex = ${Index}(_base.endIndex, nil)
I've been curious about why constant properties are still defined as computed, like https://github.com/apple/swift/blob/master/stdlib/public/core/CollectionOfOne.swift#L46
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.
Oh, I don't think that we want to add more storage to our lazy views for such a cached property.
We in general prefer var { get }
over let
because let
can require storage for the cache, and it gives more guarantees than var
, which is important for library evolution. We can't replace a let
with a var { get }
in future.
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.
Ah, I wasn't thinking about storage. That makes sense, as does the question of flexibility, thanks. In that case I think the current implementation is the right one.
New indexing model: update FlattenCollections
@natecook1000 Thank you, merged! |
This makes
.flatten()
work again for collections. I haven't turned on any tests yet since they seem so interconnected still. Are the default implementations for advance/distance okay here? They could be sped up for random-access collections, but for the others I think it's a wash.