#4342: Review containers changes
----------------------------------+-----------------------------------------
Reporter: igloo | Owner:
Type: bug | Status: new
Priority: high | Milestone: 7.2.1
Component: libraries (other) | Version: 7.1
Keywords: | Testcase:
Blockedby: | Difficulty:
Os: Unknown/Multiple | Blocking:
Architecture: Unknown/Multiple | Failure: None/Unknown
----------------------------------+-----------------------------------------
Comment(by igloo):
Thanks Milan!
T1969 and T3294 are passing now, and T3064 is better: was:
{{{
bytes allocated 316528584 is more than maximum allowed 280000000
}}}
now:
{{{
bytes allocated 299224968 is more than maximum allowed 280000000
}}}
This patch causes a lot of warnings.
I started fixing them, but in some cases I was fixing shadowed variable
warnings by SATing functions, and I'm not sure if you deliberately left
them unSATed; certainly in some cases functions seem to have been
deliberately unSATed, e.g. (diffing relative to the 7.0 branch's
containers):
{{{
lookup :: Ord k => k -> Map k a -> Maybe a
-lookup k = k `seq` go
+lookup = go
where
- go Tip = Nothing
- go (Bin _ kx x l r) =
+ STRICT12(go)
+ go k Tip = Nothing
+ go k (Bin _ kx x l r) =
case compare k kx of
- LT -> go l
- GT -> go r
+ LT -> go k l
+ GT -> go k r
EQ -> Just x
}}}
We need it to be `-Wall` clean before we can apply it, though.
Incidentally, I don't like these names:
{{{
+-- Use macros to define strictness of functions.
+-- STRICTxy denotes an y-ary function strict in the x-th parameter.
+#define STRICT12(fn) fn arg _ | arg `seq` False = undefined
+#define STRICT13(fn) fn arg _ _ | arg `seq` False = undefined
+#define STRICT23(fn) fn _ arg _ | arg `seq` False = undefined
+#define STRICT24(fn) fn _ arg _ _ | arg `seq` False = undefined
}}}
I would prefer `STRICT_1_OF_2`, but personally I think we should just use
bang patterns.
This comment:
{{{
+-- The balance function is equivalent to the following:
...
+-- It is only written in such a way that every node is pattern-matched
only once.
}}}
makes me wonder if !SpecConstr should be generating the more efficient
code automatically?
In `Data.IntSet`, I don't understand the `natFromInt` stuff, and the
change in `lookup`.
I am suspicious about the extra
{{{
| nomatch x p m = False
}}}
clause in `member`, as compared to `lookup`.
There are some things I'll leave to the containers experts to review:
I haven't checked the `balance`/`balanceL`/`balanceR` changes at all
(neither the definitions, nor the calls).
I haven't checked the change of `delta` from 4 to 3.
I haven't checked the `<=` to `<` change in `join`.
I haven't checked the changes of case into let, e.g. in
`Data.IntMap.insertLookupWithKey`.
I haven't checked the `trim` changes.
I haven't checked the `union*`/`diff*`/`hedge*` changes.
--
Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/4342#comment:12>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler
_______________________________________________
Glasgow-haskell-bugs mailing list
Glasgow-haskell-bugs@haskell.org
http://www.haskell.org/mailman/listinfo/glasgow-haskell-bugs
