fix issue 17519 - RedBlackTree doesn't like const/immutable elements#5492
fix issue 17519 - RedBlackTree doesn't like const/immutable elements#5492dlang-bot merged 1 commit intodlang:stablefrom
Conversation
|
aG0aep6G
force-pushed
the
rbtree-immutable
branch
4 times, most recently
from
200ac35 to
f90767f
Compare
PetarKirov
left a comment
PetarKirov
left a comment
There was a problem hiding this comment.
Overall LGTM. After adding one line to the unittest, I'd be ok with merging.
| assert(ts.length == 5); | ||
| assert(ts.stableInsert(cast(Elem[])[7, 8, 6, 9, 10, 8]) == 5); | ||
| Elem[] elems = [7, 8, 6, 9, 10, 8]; | ||
| assert(ts.stableInsert(elems) == 5); |
There was a problem hiding this comment.
This is only a stylistic change, right?
There was a problem hiding this comment.
This is only a stylistic change, right?
It does the same, but the old version wasn't @safe.
There was a problem hiding this comment.
I see (I missed the cast(Elem[]) last time).
|
|
||
| static private Node allocate(Elem v) | ||
| { | ||
| auto result = allocate(); |
There was a problem hiding this comment.
As far as I can see, the idea behind allocate() is to abstract the allocation mechanism. However, in practice it doesn't offer any flexibility, so we can even remove it. But that's better left for another PR.
There was a problem hiding this comment.
The idea actually comes from the original library, where I had allocators.
Since this is inside the abstraction for allocation, it is OK, as once std.experimental.allocators is ready, this will get noticed.
Thanks for reviewing, I just logged in this morning to do so and approve, but you beat me to it.
There was a problem hiding this comment.
I thought as much. Yeah, allocate immediately ringed a bell about plugging std.experimental.allocators in there to me too ;)
No problem, I'm glad that I can now effectively help :)
| import std.algorithm.iteration : map; | ||
| static struct S { int* p; } | ||
| auto t3 = new RedBlackTree!(immutable S, (a, b) => *a.p < *b.p); | ||
| t3.insert([1, 2, 3].map!(x => immutable S(new int(x)))); |
There was a problem hiding this comment.
Can you add a test that verifies that front can be fetched?
There was a problem hiding this comment.
Can you add a test that verifies that front can be fetched?
Added an assert.
PetarKirov
force-pushed
the
rbtree-immutable
branch
from
9a27dee to
29c7f36
Compare
|
I've rebased the PR in order to squash the last commit and to restart CircleCI. |
|
We're good on all testing fronts, so I'll go ahead and merge this. |
4 participants
No description provided.