Skip to content

std.container.rbtree: Fix -dip1000 compilable issues #6332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
carblue wants to merge 2 commits into from
Closed

std.container.rbtree: Fix -dip1000 compilable issues #6332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d84842
std.container.rbtree: Fix -dip1000 compilable issues
Mar 23, 2018
141bb8e
std.container.rbtree: Fix -dip1000 compilable issues; space after cast
Mar 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dip1000.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@JackStouffer JackStouffer Mar 23, 2018

Choose a reason for hiding this comment

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

the comment doesn't really need to be there.

Copy link
Contributor Author

@carblue carblue Mar 23, 2018

Choose a reason for hiding this comment

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

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.

aa[std.container.slist]=-dip25 # -dip1000 -version=DIP1000 depends on https://github.com/dlang/phobos/pull/6295 merged
aa[std.container.util]=-dip25 # TODO

Expand Down
80 changes: 76 additions & 4 deletions std/container/rbtree.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1724,10 +1724,82 @@ 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);
foreach (e; elems)
Copy link
Contributor

@JackStouffer JackStouffer Mar 23, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@schveiguy schveiguy Mar 23, 2018

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@carblue carblue Mar 23, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@JackStouffer JackStouffer Mar 23, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@schveiguy schveiguy Mar 23, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@wilzbach wilzbach Mar 28, 2018

Choose a reason for hiding this comment

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

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:

https://github.com/wilzbach/phobos/blob/012395e4dcb95b3c09b9e5d04d33871963c6b65d/std/container/rbtree.d#L934-L953

Tuple isn't scope either.

Copy link
Contributor Author

@carblue carblue Mar 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@wilzbach wilzbach Mar 28, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@wilzbach wilzbach Mar 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

For reference, the closest I got was this - it "only" requires to wrongly commenting out two lines:

Details 
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)

Copy link
Contributor

@wilzbach wilzbach Mar 28, 2018

Choose a reason for hiding this comment

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

I decided to submit this patch as new PR, s.t. it's easier to look at it and improve upon it: #6362

{
Node result;
static if (!allowDuplicates)
bool added = true;

if (!_end.left)
{
_end.left = _begin = result = allocate(e);
}
else
{
Node newParent = _end.left;
Node nxt;
while (true)
{
if (_less(e, newParent.value))
{
nxt = newParent.left;
if (nxt is null)
{
//
// add to right of new parent
//
newParent.left = result = allocate(e);
break;
}
}
else
{
static if (!allowDuplicates)
{
if (!_less(newParent.value, e))
{
result = newParent;
added = false;
break;
}
}
nxt = newParent.right;
if (nxt is null)
{
//
// add to right of new parent
//
newParent.right = result = allocate(e);
break;
}
}
newParent = nxt;
}
if (_begin.left)
_begin = _begin.left;
}

static if (allowDuplicates)
{
result.setColor(_end);
debug(RBDoChecks)
check();
++_length;
}
else
{
if (added)
{
++_length;
result.setColor(_end);
}
debug(RBDoChecks)
check();
}
}
}

/**
Expand Down Expand Up @@ -2049,7 +2121,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)
Expand Down