Fix Issue 15860 - lockstep should support foreach_reverse

[Biotronic]

This fixes https://issues.dlang.org/show_bug.cgi?id=15860 by adding opApplyReverse to Lockstep.

[dlang-bot]

Fix	Bugzilla	Description
✓	15860	lockstep should support foreach_reverse

[rcorre]

This test doesn't actually prove the iteration is going in reverse.

[DmitryOlshansky]

LGTM

[wilzbach]

Why can't we support the shortest? It's just min

[Biotronic]

Mostly just because it'd be wrong. Consider:
auto a = [1,2];
auto b = [3];
foreach_reverse (i, e1, e2; lockstep(a, b)) {
assert(e1 == a[i]); // passes
assert(E2 == b[i]); // fails
}

[wilzbach]

If you go with shortest, II expect I to be zero in the first and only iteration. Or am I missing something?

For the bidirectional ranges you have to call popBack a couple of times, with random access it's even easier.

[Biotronic]

Ah right, I misunderstood. So instead we just skip a few elements. I guess that works (it does mirror the behavior when iterating forward). The problem I see is that's an unknown cost. When iterating forward, skipping the remaining items is free. If requireSameLength had been the default, I'd be more positive, but currently that'd be a hidden cost of unknown size.

[wilzbach]

but currently that'd be a hidden cost of unknown size.

yep but only for bidirectional ranges + foreach_reverse.
I would exactly expect this behavior - it's a lot better than throwing an error imho.

[DmitryOlshansky]

That leaves off the 1 from the first array. If that's not an array, but e.g. some bidirectional generator, I'd still expect it to iterate from the end forward. Since backward iteration without an index doesn't require hasLength, it's not possible to have the behavior you describe without the index. So either the behavior is different when using an index, or we simply disallow the cases where the behavior would differ

Also would prefer current behaviour.

[wilzbach]

Ping @Biotronic - do you mind to update your pr to support shortest according to the current behavior?

[Biotronic]

Uhm... we just agreed on the polar opposite of that. Supporting indexed reverse iteration with shortest will either:

• break symmetry with regular foreach_reverse (indexes would be wrong). Or
• incur a hidden cost to skip trailing elements.

I am completely unwilling to violate the first of these, and very reluctant to break the latter.

For non-indexed foreach_reverse, shortest is already supported and behaves the way you want:

foreach_reverse (e1, e2; lockstep([1,2,3], [4,5])) {
    // Will iterate over (3,5) and (2,4) in that order.
}

[wilzbach]

This is the current behavior:

foreach_reverse (e1, e2; lockstep([1,2,3], [4,5])) {
assert(e1 == e2 - 2);
}

I guess that's why I got confused.
@DmitryOlshansky did you mean with "Also would prefer current behaviour." disallowing "shortest"?
I think I will just stop annoying you - I am wasting your time on such a rare edge case that can also be solved easier e.g. with retro - s.t. it took a longer time for its lack even to be discovered.

break symmetry with regular foreach_reverse (indexes would be wrong)

foreach_reverse (i, e1, e2; lockstep([1,2,3], [4,5])) {
  // (2,3,5), (1, 4, 2)
}

For non-indexed foreach_reverse, shortest is already supported and behaves the way you want:

Test please ;-)

[Biotronic]

Test please ;-)

A test has been added.

break symmetry with regular foreach_reverse (indexes would be wrong)

foreach_reverse (i, e1, e2; lockstep([1,2,3], [4,5])) {
  // (2,3,5), (1, 4, 2)
}

Yup, that breaks indexing - 5 is not element 2 in the array [4,5]. Writing it slightly differently, both asserts below should hold:

auto a = [1,2,3];
auto b = [4,5];
foreach_reverse(i, e1, e2; lockstep(a,b)) {
    assert(e1 == a[i]);
    assert(e2 == b[i]);
}

I think I will just stop annoying you

Please, you're not annoying - I am very glad you sparked the discussion, as it made me consider alternative solutions. All solutions have trade-offs, and I feel the ones I've chosen best fit the D philosophy, but that is certainly debatable.

I've given some of the reasons I've chosen what I have, above. Another reason is that anyone can easily add indexing with iota or an explicit counter variable - this way it's an opt-in cost, and you have to consider the trade-offs yourself.

Lastly, there is no "intuitive" way for it to behave, so the default behavior would confuse some portion of users no matter what. Throwing an exception is not optimal, but at least it gives an understandable error message instead of iterating over a different set of values than the user intended.

[Biotronic]

Finally added some documentation of the new behavior.

[wilzbach]

"an" exception ;-)

[wilzbach]

can't you just compare with n as index like in your examples? Looks a bit weird this way

[Biotronic]

Done. Sorry for the wait. :p

[wilzbach]

I feel the ones I've chosen best fit the D philosophy, but that is certainly debatable.

I think I agree now ;-)
Let's go with it!

[DmitryOlshansky]

Sounds good let's roll

[DmitryOlshansky]

Auto-merge toggled on