Add NFData1 and NFData2 classes #21

phadej · 2016-10-11T06:27:03Z

Resolves #8

TODO:

changelog
rest of tuple instance
question: what to do with non-parametric instances, like Ratio pre GHC 8 (numerator and denominator require Integral)
generic derivation of NFData1

phadej · 2016-10-11T06:48:54Z

GHC 7.8.3 build couldn't install GHC

RyanGlScott · 2016-10-16T15:56:12Z

Having generics support for NFData1 would be nice. You already have most of the machinery—you'd just need something like ToArgs (from aeson) to thread through the function argument to liftRnf, and add cases for the Generic1–specific representation types where necessary.

And to answer your question of what to do with Ratio/Complex: I think it'd be fine to define them conditionally, and make a note of this in their Haddocks.

RyanGlScott · 2016-11-17T14:41:00Z

Three weeks have passed since https://mail.haskell.org/pipermail/libraries/2016-October/027385.html, and there were no objections, so I see no problem with moving forward with this. Make sure to address the checklist in #21 (comment), as well as my comments in #21 (comment).

phadej · 2016-11-17T20:53:39Z

Only generic derivation is left.

phadej · 2016-11-17T21:55:25Z

And Generic1 => NFData1 is done too.

phadej · 2016-11-17T22:53:05Z

And finally green (I hope).

RyanGlScott

Looking good. I've left some feedback in inline comments.

RyanGlScott · 2016-11-18T15:30:55Z

tests/Main.hs

@@ -144,9 +156,32 @@ case_4_4 = testCase "Case4.4" $ withSeqState 0xffffffffffffffff $ do
    genCase n | n > 1      = Case4c (SeqSet n) (genCase (n-1))
              | otherwise  = Case4b (SeqSet 0) (SeqSet 1)

+#if __GLASGOW_HASKELL__ >= 706
+case_4_1b = testCase "Case4.1b" $ withSeqState 0x0 $ do
+    evaluate $ rnf1 $ (Case4a :: Case4 SeqSet)


A minor point, but there's quite a bit of code re-use between the case_4*s and the case_4*bs. It'd be nice to split out the values that get rnf'd into separate top-level bindings.

won't do now. As there's the hack, and luckily GHC don't float the definitions, and test break. (That's the reason why I cannot bundle them, and must have separate test cases)

RyanGlScott · 2016-11-18T15:31:02Z

Control/DeepSeq.hs

-    rnf = rnf . getProduct
+instance NFData a => NFData (Product a) where rnf = rnf1
+instance NFData1 Product where
+    liftRnf r (Product x) = r x

 -- |@since 1.4.0.0
 instance NFData (StableName a) where


NFData1 StableName?

NFData1 IORef?

NFData1 (STRef s)/NFData2 STRef?

NFData1 MVar?

NFData1 Ptr?

NFData1 ForeignPtr?

NFData2 STRef doesn't make sense, or does it?

It's well kinded, at least, as STRef as two type parameters of kind *.

RyanGlScott · 2016-11-18T15:31:05Z

Control/DeepSeq.hs

+instance NFData a => NFData1 (Const a) where
+    liftRnf _ = rnf . getConst
+instance NFData2 Const where
+    liftRnf2 r _ = r . getConst

 #if __GLASGOW_HASKELL__ >= 711
 instance (NFData a, NFData b) => NFData (Array a b) where


NFData1 (Array a)?

RyanGlScott · 2016-11-18T15:31:08Z

Control/DeepSeq.hs

@@ -355,7 +406,11 @@ instance NFData (a -> b) where rnf = rwhnf



This might be a contentious point (given the existence of #16), but there could be NFData1 ((->) a) and NFData2 (->) instances.

RyanGlScott · 2016-11-18T15:31:11Z

Control/DeepSeq.hs

@@ -327,12 +373,16 @@ instance NFData Word64   where rnf = rwhnf
 #if MIN_VERSION_base(4,7,0)
 -- |@since 1.4.0.0
 instance NFData (Proxy a) where rnf Proxy = ()
+instance NFData1 Proxy    where liftRnf _ Proxy = ()


Make sure to add @since annotations for each of these new instances.

I'm not sure if it's necessary, as that's implied by the fact NFData1 didn't exist before 1.4.3.0 anyway. But I'll add them anyway.

RyanGlScott · 2016-11-18T15:31:13Z

Control/DeepSeq.hs

+-- | A class of functors that can be fully evaluated.
+--
+-- @since 1.4.3.0
+class NFData1 f where


It'd be nice to have some documentation describing how to derive NFData1 generically, similarly to the current documentation that NFData has here.

RyanGlScott · 2016-11-20T01:52:24Z

Control/DeepSeq.hs

+instance NFData (Ptr a) where
+    rnf = rwhnf
+-- |@since 1.4.3.0
+instance NFData1 MVar where


I think if you changed this to instance NFData1 Ptr, the Travis build would work again ;)

hvr · 2016-11-20T18:28:51Z

Control/DeepSeq.hs

@@ -3,6 +3,9 @@
 #if __GLASGOW_HASKELL__ >= 702


FYI, I'm seriously considering dropping support for GHC < 7.4 with the next deepseq release to get rid of all this CPP cruft

If's up to CLC I guess?

Well, formally I'm the maintainer of deepseq, c.f. https://wiki.haskell.org/Library_submissions#The_Libraries

That being said, if the CLC objects I'd listen :-)

RyanGlScott

LGTM.

phadej mentioned this pull request Oct 11, 2016

Added deepseq and NFData instances ekmett/bound#36

Closed

hvr modified the milestone: 1.4.3 Nov 17, 2016

phadej added 3 commits November 17, 2016 22:30

Add Changelog entries

2540630

Add NFData1 and NFData2 classes

d5ebfb8

Add Changelog for NFData1/2

bdb04f3

phadej force-pushed the nfdata1 branch from aa1ba89 to df099b5 Compare November 17, 2016 20:53

phadej added 2 commits November 17, 2016 23:55

Generate tuple instances for NFData1/2

3b222d1

Add NFData1 Fixed and Ratio

5d4e1dd

phadej force-pushed the nfdata1 branch from b8801aa to 2eb158c Compare November 17, 2016 21:55

phadej force-pushed the nfdata1 branch from 2eb158c to 32d05b1 Compare November 17, 2016 22:19

Generic NFData1

865be12

phadej force-pushed the nfdata1 branch from 32d05b1 to 865be12 Compare November 17, 2016 22:27

RyanGlScott suggested changes Nov 18, 2016

View reviewed changes

phadej added 3 commits November 18, 2016 18:53

Add NFData1 generic deriving docs

37cbcf6

Add |@SInCE 1.4.3.0

c7ec6e9

Add NFData1/2 Array instances

cde8df3

RyanGlScott reviewed Nov 20, 2016

View reviewed changes

hvr reviewed Nov 20, 2016

View reviewed changes

*Var/*Ptr NFData1 instances

257b8eb

phadej force-pushed the nfdata1 branch from 70871ed to 257b8eb Compare November 20, 2016 18:41

RyanGlScott approved these changes Nov 20, 2016

View reviewed changes

RyanGlScott merged commit 418856a into haskell:master Nov 20, 2016

RyanGlScott mentioned this pull request Dec 12, 2016

Review API for conditionally defined items #27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NFData1 and NFData2 classes #21

Add NFData1 and NFData2 classes #21

phadej commented Oct 11, 2016 •

edited

Loading

phadej commented Oct 11, 2016

RyanGlScott commented Oct 16, 2016

RyanGlScott commented Nov 17, 2016

phadej commented Nov 17, 2016

phadej commented Nov 17, 2016 •

edited

Loading

phadej commented Nov 17, 2016

RyanGlScott left a comment

RyanGlScott Nov 18, 2016

phadej Nov 18, 2016

RyanGlScott Nov 18, 2016

phadej Nov 18, 2016

RyanGlScott Nov 18, 2016

phadej Nov 18, 2016

RyanGlScott Nov 18, 2016

phadej Nov 18, 2016

RyanGlScott Nov 18, 2016

phadej Nov 18, 2016

RyanGlScott Nov 18, 2016

phadej Nov 18, 2016

phadej Nov 18, 2016

RyanGlScott Nov 18, 2016

phadej Nov 18, 2016

RyanGlScott Nov 20, 2016

phadej Nov 20, 2016

hvr Nov 20, 2016 •

edited

Loading

phadej Nov 20, 2016

hvr Nov 20, 2016

RyanGlScott left a comment

		@@ -355,7 +406,11 @@ instance NFData (a -> b) where rnf = rwhnf

Add NFData1 and NFData2 classes #21

Add NFData1 and NFData2 classes #21

Conversation

phadej commented Oct 11, 2016 • edited Loading

phadej commented Oct 11, 2016

RyanGlScott commented Oct 16, 2016

RyanGlScott commented Nov 17, 2016

phadej commented Nov 17, 2016

phadej commented Nov 17, 2016 • edited Loading

phadej commented Nov 17, 2016

RyanGlScott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvr Nov 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanGlScott left a comment

Choose a reason for hiding this comment

phadej commented Oct 11, 2016 •

edited

Loading

phadej commented Nov 17, 2016 •

edited

Loading

hvr Nov 20, 2016 •

edited

Loading