Bug 1553 – foreach_reverse is allowed for delegates

Suggestion: do not allow to use delegates with foreach_reverse. Since, in the case of delegates, foreach_reverse is synonymous with foreach, it can be a cause of bugs, especially in the case of rewriting code that looped over a pre-built array to loop using a delegate.

I don't think it's right to make foreach_reverse crippled compared with foreach. I'm going to mark as won't fix.

Walter, did you understand the bug report correctly? My 4-year-old explanation could have been better, but what I meant is that: foreach (v; someDelegate) { ... } did the exact same thing as foreach_reverse (v; someDelegate) { ... } Someone may try to use foreach_reverse intuitively over a delegate, hoping it'll "just work", and get unexpected results (as I have 4 years ago).

Yes, I understood your point. I agree that one could make an error this way. I disagree that the solution is to remove the feature.

Why? This is clearly an accepts-invalid bug! What would a better solution be, anyway? reverse_delegate?

The compiler cannot tell what the delegate does, so there's no way it can diagnose an error.

Exactly! We can only assume that all delegates are written for forward iteration. I am having trouble understanding your problem with disabling foreach_reverse with delegates specifically. You say it'd be removing a feature, but how can it be a feature if it neither is nor can be implemented?

But why couldn't a delegate be written for reverse iteration? And if it does exactly the same thing for both foreach and foreach_reverse, I don't really see the problem. Granted, it may be a bit weird, but I don't see the bug. Though truth be told, I've been wondering whether foreach_reverse is actually supposed to be sticking around, given the general move towards ranges and the fact that foreach_reverse isn't actually mentioned in TDPL (at least, it's not in the index).

(In reply to comment #8) > But why couldn't a delegate be written for reverse iteration? So put the semantics in the delegate name, instead of expecting the user to always use the correct one of the two semantically-opposite but actually synonymous keywords. This can easily become a point of confusion, and I'm surprised I need to elaborate in so much detail why this is plain bad. What's wrong with writing it like this? foreach (v; &foo.reverseIterator) { ... } If you start writing it like this: foreach_reverse (v; &foo.reverseIterator) Sooner or later someone will forget the second "reverse", the code will look and compile right, and work wrong!

(In reply to comment #8) > But why couldn't a delegate be written for reverse iteration? Exactly. It can be. Why take this away?

(In reply to comment #10) > Exactly. It can be. Why take this away? You get the same thing by putting the semantic in the delegate name, without risking bugs and misunderstanding.

Have to chime in to agree with Vladimir here. foreach_reverse is such a special case (that is technically no longer useful), that I think it should only work on arrays. Essentially, using a delegate takes care of foreach_reverse for structs or classes: foreach(x; &obj.inReverse) and retro takes care of ranges, so all that is left is builtins. The only two builtin types that can be iterated are arrays and associative arrays. Reversing the order of iterating an associative array makes no sense at all. All that is left is arrays. I agree arrays need something that currently does not exist if we wanted to deep-six foreach_reverse, but it has no use outside arrays. I can't think of a single case where foreach_reverse should be used instead of foreach on a delegate. I'd say 100% of the time, it is an error. If you hear any complaining, I'll personally defend the choice :) I'm marking this as an enhancement, as technically foreach_reverse works as specified. If you do not think it's worth leaving open as an enhancement request, I'll leave it alone. The one good thing about this 'bug' is that it's not likely happen -- It likely happens when someone tries foreach_reverse on a delegate, finds it doesn't do what they want, and switches it back, never to be used again. I'd hazard to guess that if you implemented this fix, no code will break. Anywhere. If this is closed, I'll reopen for updating the documentation to say "using foreach_reverse on a delegate does *exactly the same thing* as using foreach on a delegate". If it won't be fixed, at least it should be documented as something you should never do ;) BTW, related is bug 2498 that would make the &obj.inReverse be writable as obj.inReverse (more pleasant to read/write):

(In reply to comment #12) > If this is closed, I'll reopen for updating the documentation to say "using > foreach_reverse on a delegate does *exactly the same thing* as using foreach on > a delegate". If it won't be fixed, at least it should be documented as > something you should never do ;) That would be fine.

*** Issue 6251 has been marked as a duplicate of this issue. ***

Reopened for D2 because of a community support of the described problem. See NG thread: http://forum.dlang.org/thread/[email protected]

Simply documenting this misfeature is not enough, and it's going to look really stubborn to see it being documented that 'foreach_reverse' does the same thing as 'foreach' in some X or Y context. Does std.range.retro accept an input range and document itself with: "If you pass an input range retro() will not actually reverse the iteration." ? That would be ridiculous, instead it verifies that you're passing a Bidirectional range at compile-time. I'm gonna CC Andrei on this one and hopefully we get a go-ahead to merge https://github.com/D-Programming-Language/dmd/pull/184.

Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/c756d9d22f4402088e3fd942a00085a2506649fe Fix Issue 1553 - foreach_reverse is allowed for delegates http://d.puremagic.com/issues/show_bug.cgi?id=1553 https://github.com/D-Programming-Language/dmd/commit/2f2aa99f0e555cdf5dfd1f3130e8450a4055f021 Merge pull request #184 from yebblies/issue1553 Issue 1553 - foreach_reverse is allowed for delegates

Pull to make it deprecated: https://github.com/D-Programming-Language/dmd/pull/3971

Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/59dda4abb8a147e59420feb8181824e6d97f2779 Issue 1553 - foreach_reverse is allowed for delegates Warning -> Deprecation https://github.com/D-Programming-Language/dmd/commit/2fa5086134542566d4c38b6876865ffbe2b3c4e1 Merge pull request #3971 from yebblies/issue1553d Issue 1553 - foreach_reverse is allowed for delegates

Commits pushed to 2.067 at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/59dda4abb8a147e59420feb8181824e6d97f2779 Issue 1553 - foreach_reverse is allowed for delegates https://github.com/D-Programming-Language/dmd/commit/2fa5086134542566d4c38b6876865ffbe2b3c4e1 Merge pull request #3971 from yebblies/issue1553d