Skip to content
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

Merged
merged 3 commits into from Mar 14, 2016

Conversation

natecook1000
Copy link
Member

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.

@gribozavr
Copy link
Collaborator

I haven't turned on any tests yet since they seem so interconnected still.

Please add a FIXME: swift-3-indexing-model about this to the library source so that we don't forget to check that tests exist and pass (or so that we write them).

// are `nil`, since `_inner` is `nil` iff `_outer == base.endIndex`.
_precondition(lhs._inner == nil && rhs._inner == nil)

return false
Copy link
Collaborator

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.

@gribozavr
Copy link
Collaborator

Since you are rewriting this code anyway, would you mind doing a few clean-ups across the file?

Internal initializers like internal init(_ base: Base) should have an underscore in the first argument label: internal init(_base: Base).

@gribozavr
Copy link
Collaborator

I think we can also add a performance optimization by providing a custom forEach() method. Could you add the implementation (and a FIXME for tests), or just the FIXME to add a custom implementation?

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) {
Copy link
Collaborator

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.

@gribozavr
Copy link
Collaborator

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.

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 advance() will be traversing each element of the inner array one by one, while a custom advance() could skip the whole inner array in O(1) by calling into its advance().

But I agree this is not that important. Please leave a FIXME(performance) and possibly file a bug.

@natecook1000
Copy link
Member Author

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)
}
Copy link
Member Author

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?

Copy link
Collaborator

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?

Copy link
Member Author

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

Copy link
Collaborator

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.

Copy link
Member Author

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.

gribozavr added a commit that referenced this pull request Mar 14, 2016
New indexing model: update FlattenCollections
@gribozavr gribozavr merged commit 1c07a20 into apple:swift-3-indexing-model Mar 14, 2016
@gribozavr
Copy link
Collaborator

@natecook1000 Thank you, merged!

@natecook1000 natecook1000 deleted the nc-index-flatten branch March 14, 2016 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants