Fix Issue 16996 - std.algorithm.remove with SwapStrategy.unstable rem…

[WalterWaldron]

…oves more entries

https://issues.dlang.org/show_bug.cgi?id=16996
#4973 (comment)

Note: since I renamed the steps variable, the split diff view is recommended.

[dlang-bot]

Fix	Bugzilla	Description
✓	16996	std.algorithm.remove with SwapStrategy.unstable removes more entries

[wilzbach]

Nice. Thanks a lot for the quick fix!
This LGTM, but given that I accidentally found this while adding trying to find simple public examples, how about adding a couple of internal tests to ensure everything is working fine?
Maybe with the famous DummyRange (there's also a "potential buggy" remove specialization for only bidirectional ranges).

Last but not least it would be great if you could annotate a unittest block with @nogc, s.t. we ensure that at least the unstable SwappingStrategy doesn't allocate.

[WalterWaldron]

This LGTM, but given that I accidentally found this while adding trying to find simple public examples, how about adding a couple of internal tests to ensure everything is working fine?

Ok, I'll add more thorough unit testing for both stable and unstable remove.

[WalterWaldron]

there's also a "potential buggy" remove specialization for only bidirectional ranges

I've added a rather extreme unittest which tests both stable and unstable versions of both predicate and index based remove, and after this fix all remove functions seem consistent.

Last but not least it would be great if you could annotate a unittest block with @nogc, s.t. we ensure that at least the unstable SwappingStrategy doesn't allocate.

Swapping strategy does not affect whether it allocates, so I included every function in the annotated unittest block.

[wilzbach]

you could also use https://dlang.org/phobos/std_algorithm_sorting.html#isStrictlyMonotonic

[wilzbach]

If you want to avoid the "red", uncovered line you could do sth. like:

import std.algorithm.searching : any, canFind;
import std.range : only;
return r.any!(e => !offsets.only.canFind!(o => o[0] <= e && e < o[1]));

[wilzbach]

FYI: (not important for now) in the near future there might be a new check that ensures that all unittests have either a @safe or a `@system attribute, because it helps to avoid regressions.

[wilzbach]

I've added a rather extreme unittest which tests both stable and unstable versions of both predicate and index based remove, and after this fix all remove functions seem consistent.

Excellent work!

(I have marked this for 2.073.0-b0 milestone)

[WalterWaldron]

Regarding your latest feedback:
I changed verify to assert on error instead of return a bool which should give 100% coverage.
I also Phobos idiom-ified all the unit test code and added the @safe attribute.

[wilzbach]

Auto-merge toggled on