test that instances for Eq and Ord agree with going via toAscList

[jwaldmann]

add test case, see #470

[treeowl]

We already have prop_ord for this. I don't see a similar prop_eq though.

[jwaldmann]

not exactly "this" (prop_ord uses toList, I think it should use toAscList #470 (comment) )

[treeowl]

Ah, yes, toAscList is a bit more correct. That said, I would be astonished if we made toList do something different. While it's true that it's not documented as producing keys in order, it's also not explicitly documented as potentially producing keys in some other order. There are almost certainly people whose code will break in horrible ways if we change that.

[jwaldmann]

How worried should I be about travis failing? I am using <> and foldMap in the benchmark, this trips up ghc-8.2.2. (How come it works with 7.6.3?)

[jwaldmann]

It's better now - travis breaks only for ghc-7.8.4, which does not have foldMap. Do I need to fix this? Do you know how?

[treeowl]

Travis has to work, yeah. But you don't have to run the detailed test on 7.8.

[treeowl]

Can you try to give the helper functions somewhat more informative names, and explain what they do in comments?

[jwaldmann]

Sorry for 7.8.4 I have difficulty testing locally: for me it's either this:

PATH=/opt/ghc/ghc-7.8.4/bin:$PATH  stack --resolver=lts-2.22  bench containers-tests:intset-benchmarks  
Stack no longer supports Cabal versions below 1.19.2,
but version 1.18.1.5 was found.
...
Error: While constructing the build plan, the following exceptions were encountered:

In the dependencies for gauge-0.2.4:
    basement is a library dependency, but the package provides no library
needed due to containers-tests-0 -> gauge-0.2.4

or that:

PATH=/opt/ghc/ghc-7.8.4/bin:$PATH cabal bench
...
[33 of 36] Compiling Data.Tree        ( src/Data/Tree.hs, dist/build/Data/Tree.o )

src/Data/Tree.hs:103:23: parse error on input ‘-- ^ @since 0.5.8’

[treeowl]

Can't you test using cabal test directly? Why the heck would there be a parse error on a comment?

[treeowl]

I haven't tried to work through it, but mixed strikes me as an odd way to organize this. Would this work better?

splitSign :: IntSet -> (IntSet,IntSet)
splitSign t@(Tip kx _)
  | kx >= 0 = (Nil, t)
  | otherwise = (t, Nil)
splitSign t@(Bin p m l r)
  -- m < 0 is the usual way to find out if we have positives and negatives (see findMax)
  | m < 0 = (r, l)
  | p < 0 = (t, Nil)
  | otherwise = (Nil, t)
splitSign Nil = (Nil, Nil)

that splits an IntSet into the negative and non-negative elements. Then

compare xs ys
  | (xsNeg, xsNonNeg) <- splitSign xs
  , (ysNeg, ysNonNeg) <- splitSign ys
  = case relate xsNeg ysNeg of
       Less -> LT
       Prefix -> if null xsNonNeg then LT else GT
       Equals -> orderingOf (relate xsNonNeg ysNonNeg)
       FlipPrefix -> if null ysNonNeg then GT else LT
       Greater -> GT

[jwaldmann]

OK I can do this - but I was hoping that if relateTop and orderingOf get inlined, it'll result in the same code.

[treeowl]

Well, the thing I'm really going for amounts to structuring relateTop more clearly. If you want to maintain the separation you describe, you can definitely do that with this structure as well. That would be totally fine by me.

[jwaldmann]

I am now (81bae9c) using the code that you proposed.

[treeowl]

Can't we play any tricks here? I have to think there's some way to use the prefixes and masks. For example, if we have

t1 = Bin 0100000 0000100 l1 r1
t2 = Bin 0010000 0001000 l2 r2

I think we can reach a conclusion immediately.

[treeowl]

In a bit more detail.... It seems to me that if p1 < p2, then t1 ≤ t2. Is that right? If so, then can we use the masks to determine (sometimes) that t1 < t2?

[jwaldmann]

I hope that this happens automatically since the compiler should inline orderingOf $ relateTipTip ..

[treeowl]

No, I mean pulling tricks in the Bin/Bin case. You can surely do the same thing you do with Tip: get lower and upper bounds for the trees, and if they don't overlap you have an answer.

[jwaldmann]

prefixes are tricky: assume wordSize is one (else multiply everthing by some large enough power of two), then fromList [2,3] has prefix 10 (binary) but fromList[3,4] has smaller prefix 0 (?)

[treeowl]

Okay, but can't you use your lower bound/upper bound calculation anyway? If the upper bound of one tree is less than the lower bound of the other, I think that's it.

[jwaldmann]

lower/upper-bound: yes this should be correct for any Tip/Bin combo:

  | succUpperbound t1 <= lowerbound t2 = Less
  | lowerbound t1 >= succUpperbound t2 = Greater

For now, this is inside the guards of some of the pattern matches but this could also be inverted (compare bounds first, and pattern match only if needed). This needs benchmarking (not today).

[jwaldmann]

I think that computing "lowerbound" inside a branch of a pattern match is better: the inliner then should be able to remove the pattern match that is in the implementation of "lowerbound" (does it really?)

I inserted the lower-upper test in the Bin/Bin case as helps to avoid recursion.

[treeowl]

The implementation changes for IntSet have been reverted to fix #783. It would be nice to reinstate them correctly.