Bug 10959 – std.algorithm.remove is highly bug-prone

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/[email protected]

+1, regardless of impact on backwards compatibility. It's a landmine, and it's bit me a few times in the past too.

-1, I disagree on all points except to rename to `std.algorithm.removeAt` and add a complimentary method which instead removes by value.

(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?

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

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

In 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);

I 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!

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

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

(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)() ?

+1 for this bug. I've to double-check remove every single time I use it.

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

An 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));

A PR to fix the landmines - https://github.com/dlang/phobos/pull/6154 It's probably too late to rename `remove` :/

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

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

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/phobos/issues/10001 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB