Use foreach instead of manual iterator loops in fullIntersection - #83
Open
cheeseng wants to merge 1 commit into
Open
Conversation
Both loops walked their receiver via `val it = iterator; while
(it.hasNext) { ... it.next() ... }`, though neither needs
iterator-specific capability — each is a single unconditional pass
with no early exit or interleaving between them. Switch both to
foreach for native per-collection traversal instead of an allocated
Iterator plus two virtual calls (hasNext/next) per element, same as
the deleted/updatedWith/splitAround refactor.
No sizeHint added: the split between onlyThis/both and the size of
onlyThat are entirely data-dependent on the overlap between this and
that, so no reliable size estimate exists for any of the three
builders in advance.
No behavior changes.
|
Well i think i would question this design - it is the same as an extension method that the original proposer was already doing, however they wanted to propose it as a more fundamental operation that can benefit from seeing the internals. having said that - i dont see how |
|
I asked ChatGPT so here is what is suggests for HAMT/CHAMP - https://chatgpt.com/share/6a638c28-31c4-83ed-8770-ace7891d9067 |
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
2 participants
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.
fullIntersection traversed both
thisandthatviaval it = iterator; while (it.hasNext) { ... it.next() ... }, eventhough neither loop relies on iterator-specific behavior — each is a
single unconditional pass over its collection, and the two loops
don't interleave or share iterator state.
Switch both to foreach, so each collection dispatches to its 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. Same change
already applied to deleted/updatedWith/splitAround.
No sizeHint added to any of the three builders (onlyThis, both,
onlyThat): the split between onlyThis and both depends entirely on
how much of
thisoverlaps withthat, and onlyThat's size dependssymmetrically on how much of
thatoverlaps withthis— neither isknowable in advance, so a size hint would risk over-allocating
whichever builder turns out small rather than helping.
The method remains O(|this| + |that|) with two O(1)-average contains
checks (one per loop) — the theoretical floor for a 3-way set
partition, since deriving onlyThat requires a full pass over
thatindependent of what the first loop over
thisalready determined.No behavior changes.