-
Notifications
You must be signed in to change notification settings - Fork 445
Make uniqued()
lazy by default
#71
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
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's a very salient consideration mentioned by @kylemacomber that most uses seem to expect an
Array
, in which case a.lazy.uniqued
design would make more sense.Additionally, this discrepancy here, where
uniqued(on:)
isn't lazy by default butuniqued
would be, seems like it invites confusion. I certainly would not expect that supplying a custom predicate would change the complexity or behavior so fundamentally, and I can see that issue arising when people make changes to their code and encounter this difference. Therefore, if it makes sense to makeuniqued
lazy, then I think the same change should be applied touniqued(on:)
.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.
Do people expect
uniqued()
to return an array, or do implementations usually simply return an array because it's easier to implement that way? I genuinely don't know the answer to this.I share your concern about the discrepancy between
uniqued()
anduniqued(on:)
, but I do think that viewing them in isolation, this change is consistent with other sequence operations in the standard library. Operations that can be lazy without having to compromise (except perhaps on the return type) typically are, even when called on a sequence not conforming toLazySequenceProtocol
.Collection
'sjoined()
andreversed()
fall into this category, and I argue thatuniqued()
does as well.uniqued(on:)
does not, because making it lazy would require the closure to be escaping and non-throwing.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.
Do any other algorithms in this library or the standard library differ in laziness depending on the presence of a custom predicate?
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 believe the pair of operations that comes closest is
joined()
being lazy andflatMap { $0 }
being eager while effectively doing the same thing. Of course they don't have similar names likeuniqued()
anduniqued(on:)
do.This is a fairly unique situation because other potential pairs lack one of the variants.
chunked(by: ==)
andcompactMap { $0 }
have no corresponding closure-less version, and operations likezip
,reversed
, andcombinations
have no versions that do take a closure.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.
Ya I think the
joined()
/flatMap { $0 }
andcompacted()
/compactMap { $0 }
(see #112) serve as good precedent of similar of lazy/eager pairs.To try to articulate the philosophy:
.lazy
for algorithms that take a closure to emphasize that (i) the closure will not run immediately and (ii) may run more than once per element in the collection, which can introduce a surprising vector for error.reversed
adapter over a plainCollection
would be absurd because looping over all of its indices would be O(n²).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.
Now that we have
OrderedSet
via the Swift Collections packages, I think it's even clearer to me thatuniqued
should be lazy... the ability to do partial computation is really the only thing (other than method call syntax) distinguishing this algorithm from just creating anOrderedSet
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.
+1