Skip to content

Comments

fix issue 13314 - BinaryHeap assumes Store has dup property #4929

Merged
andralex merged 5 commits intodlang:masterfrom
somzzz:issue_13314
Dec 7, 2016
Merged

fix issue 13314 - BinaryHeap assumes Store has dup property #4929
andralex merged 5 commits intodlang:masterfrom
somzzz:issue_13314

Conversation

@somzzz
Copy link
Contributor

@somzzz somzzz commented Dec 2, 2016

If Store does not have the dup property, then dup is not available for the BinaryHeap either.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
13314 BinaryHeap assumes Store has dup property

@WalterWaldron
Copy link
Contributor

Please add a unit test.

@andralex
Copy link
Member

andralex commented Dec 5, 2016

This is not covered, so codecov is unhappy. You'd need to add a unittest of dup and we're good to go. Thanks!

@somzzz
Copy link
Contributor Author

somzzz commented Dec 6, 2016

Done.

Also, writeln consumes the contents of a heap, is this expected behavior?
For example, this unittest fails:

    int[] a = [4, 1, 3, 2, 16, 9, 10, 14, 8, 7];
    auto heap = heapify(a);
    import std.stdio;
    writeln(heap);
    assert(heap.equal([16, 14, 10, 9, 8, 7, 4, 3, 2, 1]));

@JackStouffer
Copy link
Contributor

JackStouffer commented Dec 6, 2016

is this expected behavior

Looks like writeln is using the range interface to print the contents because it lacks a toString method (AFAIK). If so, then yes, that's expected.

@@ -201,12 +201,14 @@ Returns $(D true) if the heap is _empty, $(D false) otherwise.
Returns a duplicate of the heap. The underlying store must also
support a $(D dup) method.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a note here that this is only available if the underlying type has it.

@WalterWaldron
Copy link
Contributor

The unit test should also test that using a Store without dup compiles, e.g.:

struct S
{
   int[] a;
   @disable S dup();
   alias a this;	
}
	auto s = S([1,2]);
	auto h = s.heapify;

To catch possible future regressions on issue 13314.

@andralex
Copy link
Member

andralex commented Dec 6, 2016

Urgh, a disabled dup will pass the test. @somzzz I think you need to do the static if using __compiles instead...

@somzzz
Copy link
Contributor Author

somzzz commented Dec 6, 2016

Updated the tests. Let me know if you have other comments.

Also, for some reason StructWithDup fails to pick up dup from int[] in the example below:

    static struct StructWithDup
    {
        int[] a;
        alias a this;
    } 

So I had to define dup myself in the structure to make the asserts.

Similarly, @disable dup in StructWithoutDup without defining dup (declaration only) will always pass. In order to assert the functionality with @disable, dup has to be defined.

}

// Assert dup can be used on BinaryHeaps when Store has dup
assert(__traits(compiles, ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess here you could simply paste the code in the unittest instead of asserting it compiles.

@andralex andralex merged commit 9eb35e1 into dlang:master Dec 7, 2016
@wilzbach
Copy link
Contributor

wilzbach commented Dec 8, 2016

Is there any reason why this has five commits? Wasn't there a "squash-it-up in the end" policy? Now with the new Github PR capabilities this is even easier as the maintainer can do the squashing himself.

On the positive side the messages are just sth. like "comment", not Nazi's again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants