JuliaCollections / IterTools.jl

Common functional iterator patterns
Other
152 stars 28 forks source link

Minor fix: FlagFirst‘s state is always (false, ...) #72

Closed mschauer closed 4 years ago

mschauer commented 4 years ago

FlagFirst‘s state is always (false, ...)

codecov-io commented 4 years ago

Codecov Report

Merging #72 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #72   +/-   ##
=======================================
  Coverage   72.95%   72.95%           
=======================================
  Files           1        1           
  Lines         281      281           
=======================================
  Hits          205      205           
  Misses         76       76           
Impacted Files Coverage Δ
src/IterTools.jl 72.95% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8667d8a...abf4fbe. Read the comment docs.

mschauer commented 4 years ago

Btw. I compared your implementation

function iterate(ff::FlagFirst, state = (true, ))
    isfirst, rest = first(state), tail(state)
    elt, nextstate = @ifsomething iterate(ff.iter, rest...)
    (isfirst, elt), (isfirst & false, nextstate)
end

with one I had myself, using two functions

function Base.iterate(ff::FlagFirst)
    elt, state = @ifsomething iterate(ff.iter)
    (true, elt), state
end
function Base.iterate(t::FlagFirst, state)
    elt, state = @ifsomething iterate(ff.iter, state)
    (false, elt), state
end

and while IterTools's implementation is more demanding on the optimiser (harder to proof that a branch "if isfirst" can be removed from the body of a loop over the iterator, both create same code. In short: all is well.

iamed2 commented 4 years ago

IterTools's implementation is more demanding on the optimiser (harder to proof that a branch "if isfirst" can be removed from the body of a loop over the iterator

Yeah I think Tim Holy brought that up when I made my iterators blog. I haven't really come up with a good way to do it both compactly and without splatting, but I also haven't thought about it too hard.

mschauer commented 4 years ago

Let's change it actually into the two function form, that is easier to reason about (both for humans and compilers I guess)