Fix issue 15421 - The behaviours of the topNs differ with the bottom

[Infiltrator]

The behaviour with regard to the bottoms or right ranges of the topNs
differed in that topN(R, i) correctly moved elements around to preserve
each element, but that topN(R, R) did not and ended up with duplicate
and missing elements.

[Infiltrator]

Should I use lockstep here instead of indexing?

[JackStouffer]

Yes, in combination with a foreach and passing by ref

[JackStouffer]

I understand that this is just supposed to be a bug fix for 15421, but the fact that the only ranges that are tested are regular arrays is worrisome when this changes internal behavior. Please add a unittest that tests with the ranges in std.internal.testing.dummyrange

[andralex]

one stmt per line please

[Infiltrator]

Done.

[andralex]

plus @JackStouffer's comments. Thanks!

[Infiltrator]

I understand that this is just supposed to be a bug fix for 15421, but the fact that the only ranges that are tested are regular arrays is worrisome when this changes internal behavior.

I agree with you and I'm all for testing. One of the reasons that I like D so much is how easy it makes testing.

Please add a unittest that tests with the ranges in std.internal.testing.dummyrange

So do you want me to just make a unittest for each of the four Reference*Range ranges?

[JackStouffer]

So do you want me to just make a unittest for each of the four Reference*Range ranges?

No, it's better that each unittest block contains all of the tests of one type. Something like this, this should go above the unittest block for the bug

unittest
{
    import std.internal.testing.dummyrange;
    alias RandomRanges = AliasSeq!(
        DummyRange!(ReturnBy.Reference, Length.Yes, RangeType.Random),
        DummyRange!(ReturnBy.Value, Length.Yes, RangeType.Random),
    );
    foreach (T1; RandomRanges)
    {
        foreach (T2; AllDummyRanges) 
        {
            // tests
        }
    }
}

This way you test every type of random access range with every type of range.

[Infiltrator]

I must be doing something wrong here, but I can't get even a very simple unittest going with all the dummy ranges. Here is what I have boiled it down to:

unittest
{
    import std.internal.test.dummyrange;
    foreach (T; AllDummyRanges)
    {
        T a;
        T b;
        topN(a, b);
    }
}

But I can't get that to compile.

[JackStouffer]

Because the constraints require the first argument to be a random access range with a length. The first range in the AllDummyRanges AliasSeq is a forward range.

[Infiltrator]

Right, I was probably too eager with trying to reduce the error. But even using only the two random ranges with length as below gives opSlicing errors even without the changes in this PR.

unittest
{
    import std.internal.test.dummyrange;
    import std.meta : AliasSeq;
    alias RandomRanges = AliasSeq!(
        DummyRange!(ReturnBy.Reference, Length.Yes, RangeType.Random),
        DummyRange!(ReturnBy.Value, Length.Yes, RangeType.Random),
    );
    foreach (T; RandomRanges)
    {
        T a;
        T b;
        topN(a, b);
    }
}

std/container/binaryheap.d(150): Error: function std.internal.test.dummyrange.DummyRange!(cast(ReturnBy)0, cast(Length)0, cast(RangeType)3).DummyRange.opSlice (ulong lower, ulong upper) is not callable using argument types ()

Plus a few dozen more lines. I don't want to paste them all here so you can run the code yourself if you want to see.

BinaryHeap.acquire tries to call buildHeap(s[]) and DummyRange does not like that (it has no overload for opSlice(); it has only opSplice(lower, upper)). Should BinaryHeap support ranges without an opSlice()? Maybe using s.save instead?

[JakobOvrum]

r[] is not a range primitive, it just happens to work with slices which is the only type it's tested against. It should be:

static if (isRandomAccessRange!Store)
    buildHeap(s);
else
    buildHeap(s[]); // `s` is a container

[Infiltrator]

Okay, here's what I've got so far. I'm not sure what to do with the dup, but I'll look at that and sort after I get some sleep.

By the way, do I need to file bugs for the BinaryHeap and sort issues in bugzilla and put in seperate PRs or can I just sort them out here?

[JakobOvrum]

Perhaps instead of using store everywhere, add an internal primitive like:

static if (isRandomAccessRange!Store)
    alias range = store;
else
    auto range() @property
    {
        return store[];
    }

Then use range everywhere. However, BinaryHeap clearly needs more tests too, and at this point, it would probably be better to fix it in a separate PR. It also has a strange thing going on where it uses the package(std) symbol HeapOps from std.algorithm.sorting.

[Infiltrator]

Yeah, I was thinking something like that. But then there's functions like pop(Store store). I guess that I could just leave that one be.

[JackStouffer]

ping

[wilzbach]

ping @Infiltrator - do you have time to rebase?