std.container.rbtree: Fix -dip1000 compilable issues #6332
Conversation
|
Thanks for your pull request and interest in making D better, @carblue! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
| @@ -94,7 +94,7 @@ aa[std.container.array]=-dip1000 | |||
| aa[std.container.binaryheap]=-dip1000 | |||
| aa[std.container.dlist]=-dip1000 | |||
| aa[std.container.package]=-dip1000 | |||
| aa[std.container.rbtree]=-dip25 # DROP | |||
| aa[std.container.rbtree]=-dip1000 # merged https://github.com/dlang/phobos/pull/6332 | |||
There was a problem hiding this comment.
the comment doesn't really need to be there.
There was a problem hiding this comment.
dip1000.mak won't be there eternally; I hope, it will be superfluous in about a month.
Until then, I like the comments as quick reference, for TODOs etc.
| { | ||
| _setup(); | ||
| stableInsert(elems); | ||
| foreach (e; elems) |
There was a problem hiding this comment.
Did you just copy and paste the code from stableInsert into the function?
If this is necessary to get DIP1000 to compile, then something is very wrong.
There was a problem hiding this comment.
@JackStouffer I was about to ask the same question. Why can't we also instrument stableInsert and _add? I suppose stableInsert should auto-detect scope-ness, but _add is not a template.
There was a problem hiding this comment.
Frankly, I did expect exactly this question and I know, the fix is ugly, but:
The main problem was, that
private auto _add(Elem n)
returns a Node or a Tuple containing a Node, which raised the error:
scope variable result may not be returned (the same error, that I originally encountered when fixing slist.d).
Thus I would have to change _add or include a new overload for _add or inline a changed version.
In principle, it's copy and paste the code from stableInsert and _add, as well as the changes to _add to "virtually" return void.
Btw, the return of _add's Node is nowhere used, only the added member.
The upside of this uglyness is avoiding to copy the elems passed to ctor
There was a problem hiding this comment.
I'm pretty confident in saying that the wrong answer in any situation is code duplication, especially since you'll have to do the same thing to stable insert itself eventually.
The right answer here is to rewrite _add return size_t, because as far as I can tell, the returned Node is not actually used anywhere.
There was a problem hiding this comment.
The right answer here is to rewrite _add return size_t
Yes, this is the right answer in this case. I don't know why it's this way, as add in the original code returns a bool (telling if it was added or not). Perhaps there was a previous reason, and that has since been removed.
However, it's a bit disturbing that we wouldn't be able to make _add scope.
There was a problem hiding this comment.
because that's not so easy:
std/container/rbtree.d(1839): instantiated from here: RedBlackTree!(string, "a < b", true)
std/container/rbtree.d(1913): instantiated from here: redBlackTree!(true, string)
std/container/rbtree.d(940): Error: scope variable result may not be returned
and that's just for one of the two paths:
Tuple isn't scope either.
There was a problem hiding this comment.
@WalterBright Your proposal is basically the same what I tried already for std.container.slist (#6295; there it is function insertFront that I duplicated in the ctor to get it to compile) and it didn't work with master, but acc. to @wilzbach analysis with 2.079.
6295 got reverted therefore by #6353 and I think for a good reason (avoid code dupication).
I didn't yet try here (for _add) because of probably the same underlying problem for both slist.d and rbtree.d
There was a problem hiding this comment.
Oh and if we do scope _add(scope Elem n), we have to deal with _add's style of assigning to this:
std/container/rbtree.d(887): Error: scope variable result assigned to non-scope this._begin
std/container/rbtree.d(903): Error: scope variable result assigned to non-scope parameter newNode calling std.container.rbtree.RBNode!string.RBNode.left
std/container/rbtree.d(924): Error: scope variable result assigned to non-scope parameter newNode calling std.container.rbtree.RBNode!string.RBNode.right
std/container/rbtree.d(953): Error: scope variable result assigned to non-scope parameter _param_1 calling std.typecons.Tuple!(bool, "added", RBNode!string*, "n").Tuple.this
There was a problem hiding this comment.
For reference, the closest I got was this - it "only" requires to wrongly commenting out two lines:
diff --git a/std/container/rbtree.d b/std/container/rbtree.d
index a8945f082..e9967a764 100644
--- a/std/container/rbtree.d
+++ b/std/container/rbtree.d
@@ -937,7 +937,7 @@ if (is(typeof(binaryFun!less(T.init, T.init))))
debug(RBDoChecks)
check();
++_length;
- return result;
+ return result is null;
}
else
{
@@ -950,7 +950,7 @@ if (is(typeof(binaryFun!less(T.init, T.init))))
}
debug(RBDoChecks)
check();
- return Tuple!(bool, "added", Node, "n")(added, result);
+ return added;
}
}
@@ -1122,7 +1122,7 @@ if (is(typeof(binaryFun!less(T.init, T.init))))
*
* Complexity: $(BIGOH log(n))
*/
- size_t stableInsert(Stuff)(Stuff stuff) if (isImplicitlyConvertible!(Stuff, Elem))
+ size_t stableInsert(Stuff)(scope Stuff stuff) @safe if (isImplicitlyConvertible!(Stuff, Elem))
{
static if (allowDuplicates)
{
@@ -1131,7 +1131,7 @@ if (is(typeof(binaryFun!less(T.init, T.init))))
}
else
{
- return(_add(stuff).added ? 1 : 0);
+ return _add(stuff) ? 1 : 0;
}
}
@@ -1143,7 +1143,7 @@ if (is(typeof(binaryFun!less(T.init, T.init))))
*
* Complexity: $(BIGOH m * log(n))
*/
- size_t stableInsert(Stuff)(Stuff stuff) if (isInputRange!Stuff && isImplicitlyConvertible!(ElementType!Stuff, Elem))
+ size_t stableInsert(Stuff)(scope Stuff stuff) @safe if (isInputRange!Stuff && isImplicitlyConvertible!(ElementType!Stuff, Elem))
{
size_t result = 0;
static if (allowDuplicates)
@@ -1158,7 +1158,7 @@ if (is(typeof(binaryFun!less(T.init, T.init))))
{
foreach (e; stuff)
{
- if (_add(e).added)
+ if (_add(e))
++result;
}
}
@@ -1401,7 +1401,7 @@ assert(equal(rbt[], [5]));
}
/++ Ditto +/
- size_t removeKey(U)(U[] elems)
+ size_t removeKey(U)(scope U[] elems)
if (isImplicitlyConvertible!(U, Elem))
{
immutable lenBefore = length;
@@ -1423,7 +1423,7 @@ assert(equal(rbt[], [5]));
}
/++ Ditto +/
- size_t removeKey(Stuff)(Stuff stuff)
+ size_t removeKey(Stuff)(scope Stuff stuff)
if (isInputRange!Stuff &&
isImplicitlyConvertible!(ElementType!Stuff, Elem) &&
!isDynamicArray!Stuff)
@@ -1724,7 +1724,7 @@ assert(equal(rbt[], [5]));
* Constructor. Pass in an array of elements, or individual elements to
* initialize the tree with.
*/
- this(Elem[] elems...)
+ this(scope Elem[] elems...)
{
_setup();
stableInsert(elems);
@@ -1733,7 +1733,7 @@ assert(equal(rbt[], [5]));
/**
* Constructor. Pass in a range of elements to initialize the tree with.
*/
- this(Stuff)(Stuff stuff) if (isInputRange!Stuff && isImplicitlyConvertible!(ElementType!Stuff, Elem))
+ this(Stuff)(scope Stuff stuff) if (isInputRange!Stuff && isImplicitlyConvertible!(ElementType!Stuff, Elem))
{
_setup();
stableInsert(stuff);
@@ -1970,8 +1970,8 @@ if ( is(typeof(binaryFun!less((ElementType!Stuff).init, (ElementType!Stuff).init
auto rbt3 = redBlackTree(chain([0, 1], [7, 5]));
assert(equal(rbt3[], [0, 1, 5, 7]));
- auto rbt4 = redBlackTree(chain(["hello"], ["world"]));
- assert(equal(rbt4[], ["hello", "world"]));
+ //auto rbt4 = redBlackTree(chain(["hello"], ["world"]));
+ //assert(equal(rbt4[], ["hello", "world"]));
auto rbt5 = redBlackTree!true(chain([0, 1], [5, 7, 5]));
assert(equal(rbt5[], [0, 1, 5, 5, 7]));
@@ -2049,7 +2049,7 @@ if ( is(typeof(binaryFun!less((ElementType!Stuff).init, (ElementType!Stuff).init
@safe pure unittest
{
class C {}
- RedBlackTree!(C, "cast(void*)a < cast(void*) b") tree;
+ RedBlackTree!(C, (a,b) @trusted => cast(void*) a < cast(void*) b) tree;
}
@safe pure unittest // const/immutable elements (issue 17519)There was a problem hiding this comment.
I decided to submit this patch as new PR, s.t. it's easier to look at it and improve upon it: #6362
WalterBright
added
the
Review:Vision
|
Closing in favor of #6362 |
https://github.com/dlang/phobos/blob/master/dip1000.mak with
aa[std.container.rbtree]=-dip1000
Errors when running: make -f posix.mak std/container/rbtree.test
The fix basically applies @n8sh 's idea (#6295 from slist) to rbtree as well, manually inlining within the constructor as much as required for inferred @safe with scope (changing inlined _add 's return type to void)