http://d.puremagic.com/issues/show_bug.cgi?id=10009

           Summary: foreach_reverse and AA.byKey/byValue
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: henning@still-hidden.de


--- Comment #0 from Henning Pohl <henning@still-hidden.de> 2013-04-29 12:22:36 PDT ---
void main() {
    auto aa = [1 : 2, 2 : 3];
    foreach_reverse (e; aa.byKey()) { }
}

-----
test.d(4): Error: invalid foreach aggregate aa.byKey().state
-----

foreach works like a charm.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009


Andrej Mitrovic <andrej.mitrovich@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrej.mitrovich@gmail.com


--- Comment #1 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2013-04-29 13:04:56 PDT ---
I think foreach_reverse was only ever meant to be used with arrays (and is
scheduled for deprecations), for ranges you can use retro() from std.algorithm.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009


bearophile_hugs@eml.cc changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bearophile_hugs@eml.cc


--- Comment #2 from bearophile_hugs@eml.cc 2013-04-29 15:49:24 PDT ---
(In reply to comment #1)
> I think foreach_reverse was only ever meant to be used with arrays (and is
> scheduled for deprecations), for ranges you can use retro() from std.algorithm.

I have hundreds of usages of foreach_reverse in my D2 codebase. You aren't going to deprecate it. retro() is not nearly as efficient for arrays.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009



--- Comment #3 from bearophile_hugs@eml.cc 2013-04-29 15:59:48 PDT ---
(In reply to comment #2)

> I have hundreds of usages of foreach_reverse in my D2 codebase. You aren't going to deprecate it. retro() is not nearly as efficient for arrays.

Sorry, my brain was switched off. And the usages are probably under one hundred.

-----------------

(In reply to comment #1)
> I think foreach_reverse was only ever meant to be used with arrays (and is
> scheduled for deprecations), for ranges you can use retro() from std.algorithm.

Maybe the compiler can rewrite this:

foreach_reverse(x; someRange)

as:

foreach(x; someRange.retro)

But I don't know what problems this causes.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009



--- Comment #4 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2013-04-29 16:06:07 PDT ---
(In reply to comment #2)
> (In reply to comment #1)
> > I think foreach_reverse was only ever meant to be used with arrays (and is
> > scheduled for deprecations), for ranges you can use retro() from std.algorithm.
> 
> I have hundreds of usages of foreach_reverse in my D2 codebase. You aren't going to deprecate it. retro() is not nearly as efficient for arrays.

Why is retro less efficient though?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009


Jonathan M Davis <jmdavisProg@gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg@gmx.com


--- Comment #5 from Jonathan M Davis <jmdavisProg@gmx.com> 2013-04-29 17:16:04 PDT ---
> Why is retro less efficient though?

Probably something to do with how the decoding is done and/or some extra steps which can be skipped. I don't think that I ever investigated it in detail, but I know that I used foreach_reverse in some functions in std.string because it was definitely faster according to the benchmarks that I did, much as I would prefer to never use it.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009



--- Comment #6 from Andrej Mitrovic <andrej.mitrovich@gmail.com> 2013-05-02 04:01:37 PDT ---
Well we better decide whether or not foreach_reverse is actually scheduled for deprecation. If it is then we shouldn't add more features to it.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009


yebblies <yebblies@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yebblies@gmail.com


--- Comment #7 from yebblies <yebblies@gmail.com> 2013-05-03 03:40:45 EST ---
Leave foreach_reverse alone!

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009


Steven Schveighoffer <schveiguy@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |schveiguy@yahoo.com


--- Comment #8 from Steven Schveighoffer <schveiguy@yahoo.com> 2013-05-03 14:43:31 PDT ---
The problem I see with removing foreach_reverse is that the compiler actually rewrites it directly instead of calling a function.  Requiring to use retro when it is in std.range is not good for modularity.

In addition, foreach_reverse(x..y) is nice to use instead of some combination
of ranges.

However, I don't think foreach_reverse is valid for this use case.  An AA is only forward-traversable.  foreach_reverse is not possible, as buckets are stored as singly-linked lists.  If you want that, you need to first capture it as an array.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=10009



--- Comment #9 from bearophile_hugs@eml.cc 2013-05-03 15:59:38 PDT ---
(In reply to comment #8)

> However, I don't think foreach_reverse is valid for this use case.  An AA is only forward-traversable.  foreach_reverse is not possible, as buckets are stored as singly-linked lists.  If you want that, you need to first capture it as an array.

Yet this compiles:

import std.stdio: writeln;
void main() {
    auto aa = [1: 2, 2: 3];
    foreach (k, v; aa)
        writeln(k, " ", v);
    foreach_reverse (k, v; aa)
        writeln(k, " ", v);
}


And outputs:

1 2
2 3
1 2
2 3

The foreach seems to give the same sequence as the foreach_reverse :-)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------