Add OrderedSet class to standard lib

[kaeluka]

It's based on a binary tree, moderately well tested, not optimised.

[EliasC]

The Assert bundle is redundant. See assertTrue and assertFalse (with "see", of course I mean look them up in Desugarer.hs, since there is no documentation for it...)

[kaeluka]

If so, I'd suggest removing the native implementation. The stdlib is easier to maintain. Agree?

[EliasC]

@kaeluka: assertTrue has printf-like functionality which the bundle does not, but everything we can express in encore code rather than special cases in the compiler is a lot better I think. I support phasing out assertTrue. Since this includes changing existing test cases, maybe it makes sense to break this out into a PR of its own?

[EliasC]

When the warning system is in place, we could mark assertTrue as deprecated.

[supercooldave]

I agree with removing build-in implementations.

[kaeluka]

Ok, so if this is has passed review (has it?), I suggest we wait for plenary to be merged, then I'll rebase this on top of the new master.

[EliasC]

I only saw the Assert bundle and wanted to let you know about assertTrue. I can check the rest of the implementation shortly.

Would you separate Assert into a PR of its own?

[EliasC]

This seems like a break of abstraction. goesLeft reveals that OrderedSet uses a tree internally. I suggest replacing cmp and goesLeft by a strcmp like return value (and then moving to something enumy when we have support for this).

[kaeluka]

will do

[kaeluka]

@EliasC: I reacted to your inline comments and the leak of abstraction issue. Pulling out Assert into an extra PR could be done, but since that would be only 14 lines in total and this PR depends on it, I don't see a big benefit.

[EliasC]

that would be only 14 lines in total

It would be good to also update the existing tests that use assertTrue (although that could be a separate PR as well)

[kaeluka]

That has nothing to do with the Assert lib. Should be a separate PR.

[EliasC]

I would say changing test cases to use the Assert lib has something to do with the Assert lib, but let's leave it for a later PR anyway.

[EliasC]

There's one thing in the iterator interface I would like to fix. Consider the following program:

class Main
  def main() : void
    let lt = \ (x : int, y : int) -> x < y
        eq = \ (x : int, y : int) -> x == y
        set = new OrderedSet<int>(lt, eq)
    in{
      for i in [1..10]{
        set.add(i);
      };
      let iter = set.iter()
      in{
        while iter.hasNext(){
          print iter.getCurrent();
          iter.step();
        };
      }
    }

In the last loop, iter.hasNext() checks if there is a next element, not if there is a current element. This means that the loop prints the numbers 1 through 9. To print the last number we need a call to getCurrent outside of the loop. I think hasNext should be called hasMore and return true iff calling getCurrent is possible (probably a check like this.current != null).

[EliasC]

If you're not going to separate the Assert lib into a PR of its own (which I still think you should), could you at least put it into a commit of its own?

[kaeluka]

Sure, if you show me how! :)

[EliasC]

Will do! :)

[TobiasWrigstad]

Please put it in a PR of its own. I went hunting for it this morning and could not find it, because I was not expecting to find it hidden in something labelled "ordered set".

[kaeluka]

I'll do that when I'll get around to it, after OOPSLA most likely.

[TobiasWrigstad]

On a related note. There are 80+ uses of assertFalse and assertTrue in the stdlib tests. That will need refactoring before we remove anything. I am not a huge fan of removing something which has more features so casually.

[kaeluka]

See discussion above!

[kaeluka]

I now removed the assert lib from this PR.

[kaeluka]

@EliasC: Is there anything missing now?

[albertnetymk]

It feels odd to deviate from the main stream approaches Java and C# on this well-understood concept. Making for-loop to support enumerable or iterable natively (for e in ...) would get rid of all the ambiguity, but requires some change to the compiler itself, I think.

An pragmatic approach seems to use the Java convention (hasNext and next) for now, IMO.

[EliasC]

This looks good now! Will merge next time I see it :)