[GHC] #4342: Review containers changes
simonpj at microsoft.com
Wed Nov 3 06:10:46 EDT 2010
Narrowing cc list slightly to avoid span
| Bang patterns are not in any standards, so this would rule out other
| compilers not supporting them. Your rename seems reasonable, I would go
| with that.
Perhaps add a comment in the code to explain this point?
| The natFromInt && friends are the same, just with explicit INLINEs (at
| some point it did not get inlined and that caused a lot of problems).
Can you add a comment at the INLINE pragma to explain why the pragma is so important? You know, but in two years time someone looking at the code won't know.
| > | nomatch x p m = False
| > }}}
| > clause in `member`, as compared to `lookup`.
| This is as it was in 0.3. It is semantically correct both ways. The only
| think is time complexity. The member is a bit faster if the key looked for
| is _not_ in the set, but it does slightly more work if the key is present.
| We can decide both ways and make it consistent.
Again, please add a comment to explain this point. You know, but in two years time someone else is going to be puzzled.
You'll notice a common pattern to my suggestions :-). Twenty years experience with GHC has taught me to be scrupulous about documenting tricky points in the code. I have taken ot using the "Note [blah]" style a lot, because it avoids cluttering the code itself.
More information about the Cvs-ghc