vincenthz
SocksConf and NFData instances
@vincenthz
please re-open a (new) bug is something is missing or not strict enough.
You forgot to add instances for SocksConf. And also about NFData.
It's not clear why you'ld want that. with the strict annotation, seq should do the same job, there's no recursive structure or stuff to traverse. Also for variant types (e.g. SocksVersion), there's absolute nothing that need to be deepseq.
Please explain why using seq is not enough for you, or even why the strict annotation doesn't cover your problem already.
(general stuff: please don't put version bump in pull requests)
@vincenthz
It's not clear why you'ld want that. with the strict annotation, seq should do the same job, there's no recursive structure or stuff to traverse. Also for variant types (e.g. SocksVersion), there's absolute nothing that need to be deepseq.
You miss the big picture, both deriving NFData instances with generic-deepseq and actually using it to, e.g. evaluate a list of proxies, require that Socks types be instances of NFData. Just because the type is «naturally strict», doesn't mean it shouldn't have an NFData instance. This is obvious from NFData's default instances which include strict types like Bool and () http://hackage.haskell.org/packages/archive/deepseq/latest/doc/html/Control-DeepSeq.html#t:NFData.
(general stuff: please don't put version bump in pull requests)
Ok.
What I mean is some type doesn't make sense as instance of nfdata, because of the strictness of the fields, you can set NFData only on the one that are actually going to be used, not the one internally (which either doesn't make sense like SocksMethod, and SocksVer, or are already completly strict anyway like SockHostAddr). For Bool/Char/etc i can understand that it's good for autoderiving as those types could be found everywhere, but i have some doubt about some Socks type in random structure :-)
For example, I think SocksConf NFData instance that you wrote:
instance NFData SocksConf where
rnf (SocksConf s v) = s `deepseq` v `deepseq` ()
could be (equivalently):
instance NFData SocksConf where
rnf s = s `seq` ()
If i'm right, can you re-do your pull requests to do nfdata only on necessary types ?
For example, I think SocksConf NFData instance that you wrote could be (equivalently):
SocksConf fields are non-strict, so no. deepseq could be replaced by seq there, but I don't think that's important.
What I mean is some type doesn't make sense as instance of nfdata, because of the strictness of the fields, you can set NFData only on the one that are actually going to be used, not the one internally (which either doesn't make sense like SocksMethod, and SocksVer, or are already completly strict anyway like SockHostAddr). For Bool/Char/etc i can understand that it's good for autoderiving as those types could be found everywhere, but i have some doubt about some Socks type in random structure :-)
I think it's reasonable to have instances for all exported types.
rnf s = s `seq` ()
Would be insufficient because the two fields of SocksConf are not strict. Instead you probably want.
rnf (SocksConf s v) = s `seq` v `seq` ()
because all of the fields of s are strict and v is a single constructor with no fields
Alternatively ! could be added to the two fields of SocksConf
Yes, i forgot the seq on v indeed. I think i'ld just rather set the fields strict directly in the structure with the !. There's no benefits in having lazy fields here