std.algorithm.remove: Fix constraints and change popBack loop to popBackN

[Safety0ff]

See also: https://issues.dlang.org/show_bug.cgi?id=8930
Remove with swap strategy stable requires bidirectional range for popback.

[DmitryOlshansky]

So it doesn't solve issue 8930 but patches the constraints. Anyhow a net progress, so LGTM.

[monarchdodra]

s/popBackN/popBackExactly/

popBackN will actually pop "up to" N elements, checking for empty in between each call.

[Safety0ff]

Change all of the pop____Ns to pop____Exactlys? Note that the parameter is derived from input, so erroneous input will cause undefined behaviour.
EDIT: I suppose we can use the other popBack's to validate calling popBackExactly

[monarchdodra]

Yes, change pop____N to pop____Exactly.

Yes, it will cause undefined behavior, but the input is erroneous anyways.

[monarchdodra]

Well, arguably, you don't need lvalue elements to be able to swap. For example swapFront will do a value swap if there is not lvalue access. I had started working on a fix, but got sidetracked with other issues.

I perfectly fine with constraining the function to only accept what it actually works on, and we can relax them later once we have a better working implementation.

[Safety0ff]

I perfectly fine with constraining the function to only accept what it actually works on, and we can relax them later once we have a better working implementation.

This was my line of thinking, btw I'm not interested fixing 8930.

[Safety0ff]

@monarchdodra ping, could you please answer my questions?

[Safety0ff]

@DmitryOlshansky Could we please just move forward with this? If somebody wants to optimize with pop___Exactly later on, they can do so.

[monarchdodra]

@DmitryOlshansky Could we please just move forward with this? If somebody wants to optimize with pop___Exactly later on, they can do so.

Sorry I didn't see this any earlier. Just do it now, and we're good to go.

[Safety0ff]

Ok rebased on top of master and added a commit for the change from pop___N to pop____Exactly.

Note that I changed the pop____N's in the unstable version as well. The first commit only touched the constraints for the unstable version.

[DmitryOlshansky]

Auto-merge toggled on

[Safety0ff]

Thanks!

[monarchdodra]

Hey, sorry I wasn't that proactive reviewing this pull. Things have been difficult for me recently IRL. Thanks for sticking with this.

[Safety0ff]

Hey, sorry I wasn't that proactive reviewing this pull. Things have been difficult for me recently IRL. Thanks for sticking with this.

Don't worry about it. :)
Know you and a few other people carry a large amount of weight for this project and I appreciate what you do.
I also understand that oftentimes PR's get abandoned by those who initiate them.
My impatience stemmed more from the fact that we have a considerable amount of people willing to spend hours going around in circles in the forums rather than help things move forward concretely on github.