fix issue_15421 [reloaded] - The behaviours of the topNs differ with the bottom

[somzzz]

I rebased #3865 which seems abandoned.

The test for ranges still fails unless I use the cheat to transform the range to an array before passing it to topN and sort (which defeats the purpose of the test actually). It might be a problem with DummyRange, I'm still looking into it, but ideas are welcome.

https://issues.dlang.org/show_bug.cgi?id=15421

[somzzz]

Actually, there already is an issue for what is happening with the DummyRange test:
https://issues.dlang.org/show_bug.cgi?id=13314

This code example here produces the exact same errors as the DummyRange test:

/home/teemo/phobos/std/container/binaryheap.d(208): Error: template object.dup cannot deduce function from argument types !()(RefRange!(int[])), candidates are:
/home/teemo/druntime/import/object.d(1952):        object.dup(T : V[K], K, V)(T aa)
/home/teemo/druntime/import/object.d(1988):        object.dup(T : V[K], K, V)(T* aa)
/home/teemo/druntime/import/object.d(3325):        object.dup(T)(T[] a) if (!is(const(T) : T))
/home/teemo/druntime/import/object.d(3341):        object.dup(T)(const(T)[] a) if (is(const(T) : T))
/home/teemo/phobos/std/container/binaryheap.d(204): Error: function std.container.binaryheap.BinaryHeap!(RefRange!(int[]), "a < b").BinaryHeap.dup no return exp; or assert(0); at end of function
/home/teemo/phobos/std/container/binaryheap.d(406): Error: template instance std.container.binaryheap.BinaryHeap!(RefRange!(int[]), "a < b") error instantiating
test.d(7):        instantiated from here: heapify!("a < b", RefRange!(int[]))

[dlang-bot]

Fix	Bugzilla	Description

[somzzz]

Requires #4929 for tests to pass (fixes dup errors)

[andralex]

Thanks for doing this. A few adjustments and we should be good to go.

[andralex]

OK I see what you're doing here. This works but it's inefficient because it does a bunch of assignments, many of which are unnecessary. The root cause seems to be the API conditionalInsert. How about we do this - add this method to BinaryHeap:

bool conditionalSwap(ref ElementType!Store value);

The ref is important here. The method will conditionally swap value into the heap, and will swap the "lost" element of the heap into value. Then this loop only needs to run

foreach (ref e; r2)
{
    heap.conditionalSwap(e);
}

[andralex]

There's a lot of subtle duplication here and in the similar code that follows. Why is there a need to use store if the range is random access and store[] otherwise? Is it concerning a statically-sized array thing?

Anyhow, to fix this without duplication, put the conditional in a method:

auto storeRange()
{
    static if(isRandomAccessRange!Store)
        return store[];
    else
       return store;
}

Then use storeRange throughout (and get used to write calls without parens, regardless of all answers you'll see to this comment :o)).

[andralex]

space after if. @CyberShadow do we catch that automatically yet?

[CyberShadow]

@wilzbach would know, ping

[wilzbach]

space after if. @CyberShadow do we catch that automatically yet?
@wilzbach would know, ping

We do, but our automatic style checking got disabled this autumn -> #4850

[andralex]

One more push!

[andralex]

The point here is to use the swap primitive instead of assignment (it's cheap and nothrow). So the code would go:

 if (!comp(value, _store.front)) return false; // value >= largest
swap(_store.front, value);

[andralex]

I have no idea what the behavior should be if the heap has room to grow. The problem here is we collapse together the case when the heap is full and we swap an element with another, with the case when the heap is still growing.

Let's do this for now: assert(_length == _store.length) in the code, and mention that it is a precondition that the heap is fully occupied. The advantage of this is that we later get to define this behavior if needed; whereas if we define it the wrong way now, we cannot change it later.

[somzzz]

Sounds goods, thanks!

[andralex]

OK, very nice. Could you please squash all your commits into one? Then it's ready for merging. Thanks!

[andralex]

@somzzz please rebase

[somzzz]

@andralex can you also close #3865 ? Thanks!

[wilzbach]

@somzzz you have a typo in the Git commit message. That's why the Dlang bot can't link the issue and thus automatically close it (and do other stuff with it like including it within the release log).

[somzzz]

@wilzbach is it the issue number? Is there anything I can do to change it now that the PR was merged?

[andralex]

@somzzz that's for the future, for now I closed the issue so everything's in order.