fix issue 16072 - container.binaryheap should be extendable for arrays

[wilzbach]

While trying to create an top-level unittest for this module (#4331), I realized that with an array as storage, it's not possible to insert new items, which seems like a bug.

import std.container.binaryheap;
auto q = heapify!"a > b"([1, 4, 5]);
q.insert(2); // ERROR: Cannot grow a heap

Here's the fix ;-)

[dlang-bot]

Fix	Bugzilla	Description
✓	16072	std.container.binaryheap should be extendable for arrays

[PetarKirov]

Can you add a unittest? Otherwise, LGTM.

[aG0aep6G]

Maybe insertBack should work on arrays instead?

[wilzbach]

Can you add a unittest? Otherwise, LGTM.

Done. Also added a check + fix for the case of a zero-length arrays.

Maybe insertBack should work on arrays instead?

Afaict is insertBack deliberately picked for the container API to avoid interferring with range API.

See also: http://dlang.org/phobos-prerelease/std_container.html

[aG0aep6G]

Afaict is insertBack deliberately picked for the container API to avoid interferring with range API.

I don't see how they would interfere. Arrays already support a bunch of the container primitives. Adding the rest seems more natural to me than special casing functions that work on containers.

[wilzbach]

I don't see how they would interfere. Arrays already support a bunch of the container primitives. Adding the rest seems more natural to me than special casing functions that work on containers.

I actually asked the same question before and IIRC it was about guareenting runtime based on the name (?). Ping @schveiguy

[schveiguy]

@wilzbach I have no experience with binary heap, so I don't really have any opinion on it.

[wilzbach]

@wilzbach I have no experience with binary heap, so I don't really have any opinion on it.

I thought the container API is similiar between all storages. Sorry.

I actually asked the same question before

See #4038 - ping @JackStouffer

[JackStouffer]

What if store is a slice of an array?

[schveiguy]

What if store is a slice of an array?

You mean a dynamic array? Should work.

[JackStouffer]

I guess I misunderstood how _store worked.

[JackStouffer]

This change would be cleaner if it was written in the following way

        else static if (isDynamicArray!Store)
        {
            if (_store.length == 0)
                _store.length = 10;
            if (length == _store.length)
                _store.length = length * 2;
            _store[_length] = value;
        }
        else
        {
            // can't grow
            enforce(length < _store.length,
                    "Cannot grow a heap created over a range");
        }

[wilzbach]

I didn't know the reason for setting the element after an exception was thrown, so I didn't change the code, but thanks for the heads-up -> changed :)

[andralex]

OK, interesting... wonder what the consequences of this precedent will be :)

[wilzbach]

OK, interesting... wonder what the consequences of this precedent will be :)

What consequences do you expect? I think the other containers support insertions ;-)

(rebased)

[quickfur]

@andralex So is this PR a go or no-go?

[wilzbach]

OK, interesting... wonder what the consequences of this precedent will be :)
@andralex So is this PR a go or no-go?

I would interpret the "OK, interesting" as an approval, but maybe other diviner are better in interpreting (?)

[JackStouffer]

@quickfur I see Andrei's comment as consent

I think this PR is ready to go

[andralex]

Auto-merge toggled on