Use foreach instead of manual iterator loops in Seq.deleted/updatedWith/splitAround - #82
Open
cheeseng wants to merge 1 commit into
Open
Conversation
All three methods traversed via a manual iterator/while loop even though none of them need iterator-specific capabilities (resumable position, early termination) — every element is visited regardless. Switch them to foreach, which dispatches to each collection's native traversal (e.g. List walking cons cells, index-based loops for array-backed types) instead of paying for an Iterator allocation plus two virtual calls (hasNext/next) per element. - deleted: also add b.sizeHint(this, -1), since the result size is always known exactly (one fewer than the source) when a valid index is found. - updatedWith: also add b.sizeHint(this), optimistically assuming the common case where f returns Some (same size as source); sizeHint is documented to tolerate being wrong, so a None at the matched index just means a harmless one-off overshoot. Also replace the manual Some/None match with f(a).foreach(b += _), matching the idiom used elsewhere. - splitAround: rewritten as a single boolean-flag state machine (switched) instead of two sequential iterator loops. No sizeHint added here — the split point is data-dependent, so neither builder has a reliable size estimate in advance. No behavior changes.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
deleted, updatedWith, and splitAround each traversed their receiver
via
val it = iterator; while (it.hasNext) { ... it.next() ... },even though none of them rely on iterator-specific capabilities —
every element is visited unconditionally in all three (splitAround's
two-loop shape looks like it needs a resumable iterator, but the same
logic is expressible as a single pass with a boolean flag).
Switch all three to foreach, which dispatches to each collection's
own native traversal (e.g. List walking cons cells directly,
index-based loops for array-backed types) instead of allocating an
Iterator and paying two virtual calls (hasNext/next) per element.
Additional changes:
exactly once a valid index is found (source size minus one), so the
builder can avoid backing-array resizes as elements are added.
common case where f returns Some (result size == source size).
sizeHint is documented to tolerate being wrong, so a None at the
matched index only costs a harmless one-element overshoot. Also
replaced the manual
f(a) match { case Some(v) => b += v; case None => }withf(a).foreach(b += _), matching Option.foreachusage elsewhere in this file.
switchedboolean instead of two sequential iterator loops. No sizeHint added
here, since the split position is data-dependent and there's no
reliable size estimate available for either builder in advance.
No behavior changes; existing tests should cover this as-is.