Closed tkf closed 4 years ago
Merging #148 into master will decrease coverage by
0.13%
. The diff coverage is87.50%
.
@@ Coverage Diff @@
## master #148 +/- ##
==========================================
- Coverage 92.11% 91.98% -0.14%
==========================================
Files 21 22 +1
Lines 444 449 +5
==========================================
+ Hits 409 413 +4
- Misses 35 36 +1
Impacted Files | Coverage Δ | |
---|---|---|
src/initials.jl | 75.00% <75.00%> (ø) |
|
src/base.jl | 83.15% <100.00%> (+0.17%) |
:arrow_up: |
src/collectors.jl | 100.00% <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 69d6a64...0b7d641. Read the comment docs.
Using the default approach
InitialValues.@def_monoid monoid!!
was problematic because definingmonoid!!(::CustomType, x)
introduced method ambiguities. It could be solved semi-automatically by@disambiguate append!! CustomType
but it is very ugly and very problematic for the extensibility of the method.Furthermore, the definition
that is generated by
@def_monoid
is not appropriate ifsrc
should be consumed by the timeappend!!
is called (e.g., https://discourse.julialang.org/t/38845/3). This is mostly the case since the returned value ofmonoid!!(init, src)
would be used as the first argument (i.e., it would be mutated) in the "next iteration" in reduction code idioms. That is to say,monoid!!
should not assume the ownership of the second argument.The method ambiguity problem is solved by introducing a "dispatch pipeline"; i.e., a
CustomType
implementer can overload__monoid!!__
instead ofmonoid!!
.The memory ownership problem is solved by defining InitialValues.jl interface manually. For example,
append!!(::InitAppend!!, src)
now callscopymutable(src)
.Alternative approach
For
append!!
, an alternative approach considered is to create a customEmptyCollection
type that behaves like "an" identity element ofappend!!
. This may be possible by allowingInitialValues.Init(::typeof(monoid!!))
overload. This approach has some nice properties:No need to define
append!!(::Any, ::InitType)
as it would work via the default implementation ofappend!!
based on iterator. (The flip-side is that a bug could be hidden if the init object is fed to other functions likecollect
. But this is not a serious problem.)By using the trait mechanism and defining
NoBang.append
,append!!
would behave as expected without any "surface" dispatches.A major downside of this approach is that the fold code does not have a canonical way to check if
monoid!!
is ever called. If overloadingInitialValues.Init
were supported, the fold implementers cannot simply callacc isa InitialValue
to check ifacc = monoid!!(acc, x)
was called at least once.Commit Message
Custom definition for
append!!(_, Init(append!!))
(#148)Implement a better fix for the method ambiguity problem of
append!!(_, Init(append!!))
. Also fix the ownership problem ofappend!!(Init(append!!), xs)
by copyingxs
.