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

refactor(skip+takeWhile): Make impls consistent #223

Closed
wants to merge 1 commit into from

Conversation

bartelink
Copy link
Member

Calved from #220: Cleanup/refactoring of skipWhile and takeWhile.

Largely for me to distil the implementation so I could get to a clear understanding of the difference between the Inclusive and non-Inclusive variants.

Copy link
Member

@abelbraaksma abelbraaksma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some thoughts, not a full review yet. EDIT: my review comment was bogus, I've removed it.

@abelbraaksma abelbraaksma added refactoring Cleanup, refactoring and minor fixes topic: surface area Adds functions to the public surface area labels Dec 24, 2023
@abelbraaksma abelbraaksma force-pushed the refactor-skip-take branch 2 times, most recently from cecff30 to 1e511be Compare December 29, 2023 02:22
@abelbraaksma
Copy link
Member

I spent a little time today with the code changes proposed here. In general, I'm totally in favor of cleaner code. However, there's a (sometimes) significant performance penalty associated with these changes:

let inclusive =
    match whileKind with
    | Inclusive -> true
    | Exclusive -> false

match predicate with
| Predicate predicate -> // takeWhile(Inclusive)?
    while cont do
        if predicate e.Current then
            yield e.Current
            let! hasMore = e.MoveNextAsync()
            cont <- hasMore
        else
            if inclusive then  // this is a problem. This should be lifted
                yield e.Current

            cont <- false

The original code makes the decision between inclusive/exclusive on the outer rims of the function, ensuring that these static values do not trickle down to the inner loop.

However, I am not quite happy with the current implementation either, so I'll give it another round tomorrow, while also keeping the consistency-improvements that you added here.

@bartelink
Copy link
Member Author

Thanks for all the investigation work
Please don't invest too much time in this; I and the world would much value getting out of alpha above not 'losing' my work or any such concerns
Ultimately the tests cover all this well, so the impl consistency is not truly critical

@abelbraaksma
Copy link
Member

This PR has been continued and completed in #235, using the principles shown here. It turned out, a lot more could be simplified in these functions.

@abelbraaksma abelbraaksma added this to the v0.4.0 milestone Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Cleanup, refactoring and minor fixes topic: surface area Adds functions to the public surface area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants