Closed bartelink closed 6 months ago
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.
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
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.
Calved from #220: Cleanup/refactoring of
skipWhile
andtakeWhile
.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.