Fix Issue 9362 - Add a method to remove one item to std.container.SList #5784

Darredevil · 2017-10-16T13:36:21Z

https://issues.dlang.org/show_bug.cgi?id=9362

If everyone agrees with this I will go ahead and do the same for Dlist.

dlang-bot · 2017-10-16T13:36:22Z

Thanks for your pull request, @Darredevil! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	9362	Add a method to remove one item to std.container.SList

wilzbach

A couple of nits. I agree that this can be useful, but we plan to replace std.container soonish (hopefully by the end of the year with a new std.experimental.container library). Of course, deprecating std.container (and moving forward) will take a much longer time...

wilzbach · 2017-10-16T13:45:39Z

std/container/slist.d

@@ -123,6 +123,19 @@ struct SList(T)
        return n;
    }

+    private static Node * findNodeByValue(Node * n, T value)


Nit: style: Node*

Was trying to maintain the general style of the Slist implementation. I will update it.

As mentioned in https://wiki.dlang.org/Contributing_to_Phobos:

Be critical when looking at Phobos code (some parts of Phobos are a bit older and have some relics...)

And also in Contributing_to_Phobos#Code_style:

Stick to the style guide

wilzbach · 2017-10-16T13:46:26Z

std/container/slist.d

@@ -123,6 +123,19 @@ struct SList(T)
        return n;
    }

+    private static Node * findNodeByValue(Node * n, T value)
+    {
+        assert(n);


Needs an error message, e.g. "The node can't be null"

(I know that there are other methods in this module don't do this either, but that's no reason to continue using the old style)

wilzbach · 2017-10-16T13:48:01Z

std/container/slist.d

+
+/**
+Removes an element from the list in linear time.
+


Parameter block is missing.

wilzbach · 2017-10-16T13:48:32Z

std/container/slist.d

+     */
+    void linearRemoveElement(T value)
+    {
+        enforce(_first);


Why is removing 0 invalid?

Also I think it would be better to return a boolean value - true iff the element existed and was successfully removed (false otherwise).

wilzbach · 2017-10-16T13:50:20Z

std/container/slist.d

+    a.linearRemoveElement(4);
+    assert(equal(a[], [3]));
+    a.linearRemoveElement(3);
+    assert(a.empty());
 }


Would need a test for the behavior when one value occurs multiple times.

Added test for this.

wilzbach · 2017-10-16T13:52:41Z

std/container/slist.d

+        while (ahead && ahead._payload != value)
+        {
+            n = ahead;
+            enforce(n);


Why do you test n / ahead here? The loop body is only executed if ahead is evaluated to true (i.e. not null).

wilzbach · 2017-10-16T13:54:10Z

std/container/slist.d

@@ -589,6 +602,39 @@ Complexity: $(BIGOH n)

 /// ditto
    alias stableLinearRemove = linearRemove;
+
+/**
+Removes an element from the list in linear time.


Define behavior when the value occurs multiple times (e.g. the list is iterated from front to back and the first match is removed).

For example:

Removes the first an element from the list that equals `value` in $(BIGOH n) time.

edi33416 · 2017-10-16T14:34:58Z

LGTM

PetarKirov · 2017-10-16T14:39:05Z

std/container/slist.d

+
+Complexity: $(BIGOH n)
+     */
+    void linearRemoveElement(T value)


I think it would be better to return a boolean value - true iff the element existed and was successfully removed (false otherwise).

In which case, findNodeByValue should probably return null (without asserting) if _root is also null.

Darredevil · 2017-10-17T12:51:28Z

@ZombineDev @wilzbach this PR should be ready for merge. Jenkins seems to be unhappy with something unrelated with this PR.

PetarKirov

LGTM. All of the findNode* functions could be refactored on top of range primitives if the SList(T).Range was implemented via Node**, but that's a topic for another day.

wilzbach · 2017-10-17T15:00:05Z

Being a public symbol this still needs an approval from @andralex

andralex · 2017-10-17T16:59:41Z

std/container/slist.d

@@ -123,6 +123,18 @@ struct SList(T)
        return n;
    }

+    private static Node* findNodeByValue(Node* n, T value)
+    {
+        assert(n, "Error: un-initialized List");


should return null for empty list

andralex · 2017-10-17T17:00:00Z

std/container/slist.d

+{
+    import std.algorithm.comparison : equal;
+
+    auto a = SList!int(-1, 1, 2, 1, 3, 4);


should test against an empty list

Darredevil · 2017-10-18T16:00:36Z

@andralex @ZombineDev should be ready for merge

andralex · 2017-10-18T22:45:08Z

std/container/slist.d

+    {
+        auto n1 = findNodeByValue(_root, value);
+
+        // if node exists


andralex · 2017-10-24T13:28:12Z

@wilzbach this fails the style checker, which segfaults. I'll pull this, could you please investigate the matter?

Darredevil requested a review from PetarKirov as a code owner October 16, 2017 13:36

Darredevil force-pushed the issue-9362-slist-remove branch 2 times, most recently from 39dc2bb to 5973ce7 Compare October 16, 2017 13:39

dlang-bot added the Enhancement label Oct 16, 2017

wilzbach reviewed Oct 16, 2017

View reviewed changes

Darredevil force-pushed the issue-9362-slist-remove branch from 5973ce7 to 0cdaefd Compare October 16, 2017 14:23

PetarKirov reviewed Oct 16, 2017

View reviewed changes

Darredevil force-pushed the issue-9362-slist-remove branch 2 times, most recently from b719d20 to 644716d Compare October 16, 2017 16:30

Darredevil mentioned this pull request Oct 17, 2017

Add liniarRemoveElement for Dlist #5786

Merged

Darredevil force-pushed the issue-9362-slist-remove branch 2 times, most recently from 667cd59 to ff1b70f Compare October 17, 2017 13:16

PetarKirov approved these changes Oct 17, 2017

View reviewed changes

wilzbach added the @andralex Approval from Andrei is required label Oct 17, 2017

wilzbach assigned andralex Oct 17, 2017

andralex requested changes Oct 17, 2017

View reviewed changes

andralex removed the @andralex Approval from Andrei is required label Oct 17, 2017

Darredevil force-pushed the issue-9362-slist-remove branch from ff1b70f to ff7c90d Compare October 18, 2017 15:59

andralex approved these changes Oct 18, 2017

View reviewed changes

std/container/slist.d Outdated

{

auto n1 = findNodeByValue(_root, value);

// if node exists

Copy link

Member

andralex Oct 18, 2017

Choose a reason for hiding this comment

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

delete

Fix issue 9362 - Add linearRemoveElement from Slist

aee5667

Darredevil force-pushed the issue-9362-slist-remove branch from ff7c90d to aee5667 Compare October 19, 2017 14:32

andralex merged commit e5a146b into dlang:master Oct 24, 2017

Darredevil mentioned this pull request Oct 25, 2017

Changelog update for Dlist and Slist linearRemoveElement #5808

Merged

edi33416 mentioned this pull request Oct 28, 2017

Fix segregator alignedReallocate and empty #5802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 9362 - Add a method to remove one item to std.container.SList #5784

Fix Issue 9362 - Add a method to remove one item to std.container.SList #5784

Darredevil commented Oct 16, 2017

dlang-bot commented Oct 16, 2017 •

edited

wilzbach left a comment

wilzbach Oct 16, 2017

Darredevil Oct 16, 2017

PetarKirov Oct 16, 2017 •

edited

wilzbach Oct 16, 2017

wilzbach Oct 16, 2017

wilzbach Oct 16, 2017

PetarKirov Oct 16, 2017

wilzbach Oct 16, 2017

Darredevil Oct 16, 2017

wilzbach Oct 16, 2017

wilzbach Oct 16, 2017

PetarKirov Oct 16, 2017

edi33416 commented Oct 16, 2017

PetarKirov Oct 16, 2017

PetarKirov Oct 16, 2017

Darredevil commented Oct 17, 2017

PetarKirov left a comment

wilzbach commented Oct 17, 2017

andralex Oct 17, 2017

andralex Oct 17, 2017

Darredevil commented Oct 18, 2017

andralex Oct 18, 2017

andralex commented Oct 24, 2017

Fix Issue 9362 - Add a method to remove one item to std.container.SList #5784

Fix Issue 9362 - Add a method to remove one item to std.container.SList #5784

Conversation

Darredevil commented Oct 16, 2017

dlang-bot commented Oct 16, 2017 • edited

Bugzilla references

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetarKirov Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edi33416 commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darredevil commented Oct 17, 2017

PetarKirov left a comment

Choose a reason for hiding this comment

wilzbach commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darredevil commented Oct 18, 2017

Choose a reason for hiding this comment

andralex commented Oct 24, 2017

dlang-bot commented Oct 16, 2017 •

edited

PetarKirov Oct 16, 2017 •

edited