NFData1,NFData2 instances (#767) #992
Conversation
|
This would be good to have. @dbeacham would you like to finish this PR? |
|
Happy to - I'll finish it up next week. |
dbeacham
force-pushed
the
nfdata1-nfdata2-instances
branch
2 times, most recently
from
83e4566
to
57cc685
Compare
| liftRnf _ Nil = () | ||
| liftRnf rnfx (Tip _ v) = rnfx v | ||
| liftRnf rnfx (Bin _ l r) = liftRnf rnfx l `seq` liftRnf rnfx r | ||
|
|
There was a problem hiding this comment.
Should we use a helper function to allow this to inline?
There was a problem hiding this comment.
I've added one here - but possibly there are other instances that could benefit?
Also not sure if there's any need for any INLINE or INLINABLE pragmas.
There was a problem hiding this comment.
I probably wouldn't add pragmas for this. The function is small enough that I'd expect GHC to do a decent job of deciding when to inline it. Letting others inline would probably be good too.
There was a problem hiding this comment.
Seq will be tricky to allow inlining for (it requires a GADT, or better a fake GADT, and I don't think we have one here), so don't worry too much about that.
There was a problem hiding this comment.
I think that's now done for Map, Set, IntMap and Tree.
There was a problem hiding this comment.
Seems like it would be a good to have a ()-like type with a strict <> instance, then we can write all NFData stuff simply using foldMap and foldMapWithKey instead of rewriting tree traversals every time.
There was a problem hiding this comment.
@meooow25 , that sounds clever.
There was a problem hiding this comment.
I wanted to see if this exists in/could be added to deepseq, and it seems like you made an attempt a while ago: haskell/deepseq#18 😄
It would be handy to have the types UnitLTR/UnitRTL available from deepseq, but for now we'll have to define our own if we do this.
There was a problem hiding this comment.
Opened haskell/deepseq#105
dbeacham
force-pushed
the
nfdata1-nfdata2-instances
branch
from
57cc685
to
cdb3a07
Compare
dbeacham
force-pushed
the
nfdata1-nfdata2-instances
branch
from
cdb3a07
to
6ef991c
Compare
| * `NFData1`, `NFData2` instances for `Data.Graph`, `Data.IntMap`, | ||
| `Data.IntSet`, `Data.Map`, `Data.Sequence`, `Data.Set`, `Data.Tree` and |
There was a problem hiding this comment.
Data.Graph could be misunderstood, SCC got the instance (not Graph). And IntSet is not affected.
We can also just drop the Data. and say "...for IntMap, Map, Set, Tree, SCC", I think that's good enough.
|
Speaking of changes, we usually use |
I've added
NFData1andNFData2instances where appropriate forData.Graph,Data.IntMap,Data.IntSet,Data.Map,Data.Sequence,Data.Set,Data.Treeand relevant internal dependenciesTODO:
@sincedocs