std.algorithm.remove is highly bug-prone #10001
Labels
Comments
tcdknutson commented on 2013-09-03T14:52:30Z+1, regardless of impact on backwards compatibility. It's a landmine, and it's bit me a few times in the past too. |
daniel350 commented on 2013-09-09T17:15:16Z-1, I disagree on all points except to rename to `std.algorithm.removeAt` and add a complimentary method which instead removes by value. |
bearophile_hugs commented on 2013-09-10T05:59:32Z(In reply to comment #2)
> -1, I disagree on all points except to rename to `std.algorithm.removeAt` and
> add a complimentary method which instead removes by value.
Are you saying that std.algorithm.remove is not very bug-prone? And why? |
daniel350 commented on 2013-09-10T07:28:02ZAfter 24 hours of thinking about it, I've come to agree with your statement. My original sentiment was that of likening std.algorithm.remove to its look-alike http://www.cplusplus.com/reference/algorithm/remove.
I also saw the slice level purity of the operation as being an attractive quality, however, given the majority of the range interfaces in D are mutating by default, I see no reason why the behavior of this function should be different.
To which end, I now agree on all points. Sorry, I'll hope you forgive me.
+1. |
bearophile_hugs commented on 2013-09-10T07:34:20Z(In reply to comment #4)
> Sorry, I'll hope you forgive me.
Thank you, but you don't need to ask for forgiveness for just disagreeing with me :-) Disagreeing is a natural part of discussions. |
bearophile_hugs commented on 2014-01-09T03:11:59ZIn Python to remove an item from an array (list) you use:
>>> items = [10, 20, 30, 20]
>>> items.remove(20)
>>> items
[10, 30, 20]
With D+Phobos you need:
void main() {
import std.stdio, std.algorithm;
auto items = [10, 20, 30, 20];
items = items.remove(items.countUntil(20));
items.writeln;
}
So this API is quite bad all around, it's not just bug-prone.
So I suggest to introduce a differently named function that works similarly to the current remove (but it doesn't need the bug-prone re-assignment on the left):
items.removeAtIndex(1);
And modify remove() to work like this, as the Python remove method:
items.remove(20); |
damianday commented on 2014-01-09T11:21:19ZI absolutely agree, I find this crops up all the time
when using containers, so I usually roll my own, which is a shame!
Bring on removeAtIndex! |
bearophile_hugs commented on 2014-01-22T15:33:13Z> items = items.remove(items.countUntil(needle));
monarch_dodra comments:
> Hum... that requires iterating the range twice for a non-RA
> range. And you forgot a save:
>
> items = items.remove(items.save.countUntil(needle)); |
daniel350 commented on 2014-01-22T21:33:11ZI've just given up on this idiom, instead using:
```
import std.algorithm;
import std.stdio;
void main() {
auto items = [10, 20, 30];
auto t = items.filter!(x => x != 20).copy(items);
items = items[0 .. $ - t.length];
writeln(items);
}
```
Its the only option that has clear time and space semantics.
Ref: http://dpaste.dzfl.pl/84273326 |
monarchdodra commented on 2014-01-22T23:22:14Z(In reply to comment #9)
> I've just given up on this idiom, instead using:
>
> ```
> import std.algorithm;
> import std.stdio;
>
> void main() {
> auto items = [10, 20, 30];
> auto t = items.filter!(x => x != 20).copy(items);
> items = items[0 .. $ - t.length];
>
> writeln(items);
> }
> ```
>
> Its the only option that has clear time and space semantics.
>
> Ref: http://dpaste.dzfl.pl/84273326
How is that any different from:
items = items.remove!(x => x == 20)()
? |
advmail commented on 2014-01-23T00:55:42Z+1 for this bug. I've to double-check remove every single time I use it. |
daniel350 commented on 2014-01-23T01:52:09Z(In reply to comment #10)
> (In reply to comment #9)
> > I've just given up on this idiom, instead using:
> >
> > ```
> > import std.algorithm;
> > import std.stdio;
> >
> > void main() {
> > auto items = [10, 20, 30];
> > auto t = items.filter!(x => x != 20).copy(items);
> > items = items[0 .. $ - t.length];
> >
> > writeln(items);
> > }
> > ```
> >
> > Its the only option that has clear time and space semantics.
> >
> > Ref: http://dpaste.dzfl.pl/84273326
>
> How is that any different from:
> items = items.remove!(x => x == 20)()
> ?
A fair point, I'd forgotten about the predicate remove. |
bearophile_hugs commented on 2014-03-16T08:31:34ZAn extra feature of the new "removeIndex" (or "removeAtIndex") function is to optionally accept a range of indexes:
myArray = [.....];
myArray.removeIndex(2);
myArray.removeIndex(iota(1, 20, 3)); |
greensunny12 commented on 2018-02-10T22:09:30ZA PR to fix the landmines - https://github.com/dlang/phobos/pull/6154
It's probably too late to rename `remove` :/ |
mingwu commented on 2024-08-11T06:01:02ZHit this one today,
Has `removeAt` or `removeAtIndex` be added to the std lib?
BTW, for associative array, `remove()` is in-place; but here for std.algorithm.mutation.remove, one need to do
```
array = array.remove(index); // return a new container
// v.s.
aa.remove(key); // return bool (if it's removed)
```
This in-consistence is really bad. |
ajv-271-109-6446 commented on 2024-08-21T13:33:13Z(In reply to Seb from comment #14)
> A PR to fix the landmines - https://github.com/dlang/phobos/pull/6154
>
> It's probably too late to rename `remove` :/
I'd suggest deprecating it, and adding removeAt and removeValue
It is certainly, as several people have commented, a landmine. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
bearophile_hugs reported this on 2013-09-03T14:47:27Z
Transfered from https://issues.dlang.org/show_bug.cgi?id=10959
CC List
Description
This is not an enhancement request, I consider this a bug report. Often when I use std.algorithm.remove in my code I introduce bugs. So I believe std.algorithm.remove is too much bug-prone, some examples: import std.algorithm: remove; import std.stdio: writeln; void main() { auto A = [1, 2, 3]; A.remove(2); writeln(A); // prints: [1, 2, 3] A.remove!q{a = 2}; writeln(A); // prints: [1, 2, 3] A.remove!q{a == 2}; writeln(A); // prints: [1, 3, 3] A = [1, 2, 3]; A = A.remove!q{a == 2}; writeln(A); // prints: [1, 3] (correct) } So I suggest to rename std.algorithm.remove as "removeAtIndex" or something similar. And then to introduce a function remove that removes the given item. But this is not enough, because even this syntax is bug-prone to remove the item '2' from the array 'A': A = A.remove(2); What I should write is just: A.remove(2); This is how it's done in Python, and it's not bug-prone, this is how in my opinion it should be designed: >>> A = [1, 2, 3] >>> A.remove(2) >>> A [1, 3] I don't care about all the discussions about containers, ranges, etc. Currently std.algorithm.remove is a landmine and in my opinion it's not acceptable in Phobos. See also a thread by Manu and friends, that have had problems to remove an array item: http://forum.dlang.org/thread/mailman.680.1378001151.1719.digitalmars-d@puremagic.comThe text was updated successfully, but these errors were encountered: