On Wednesday, 23 October 2019 at 09:27:32 UTC, Walter Bright wrote:
>
> I suggest:
>
> 1. what it does now
> 2. what is wrong with it
> 3. what the proposed change is
>
> I'd also suggest making it clear that this proposal does not affect static foreach. Discussion of static foreach should be removed because its length implies that the proposal is making changes to it, and explaining what static foreach does is therefore off topic for the DIP.

Okay, Sounds doable.

On Wednesday, 23 October 2019 at 09:07:24 UTC, Andrea Fontana wrote:
>
> I don't think so. But I can be wrong about this. I don't think I was trying to push a breaking change in dmd also because that would broke the whole phobos each!T implementation.

So I'll have to look what Andrei commented on then. Perhaps he wasn't discussing even what I thought he was, in which case there is no reason to keep the citation on the DIP.

On 19.10.19 15:16, Mike Parker wrote:
> This is the feedback thread for the second round of Community Review for DIP 1022, "foreach auto ref":
> 
> https://github.com/dlang/DIPs/blob/089816bc47ee3d1df06d10aa2af943d3e70e6161/DIPs/DIP1022.md 
> 
> 
> All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on November 2, or when I make a post declaring it complete.
> 
> At the end of Round 2, if further review is deemed necessary, the DIP will be scheduled for another round of Community Review. Otherwise, it will be queued for the Final Review and Formal Assessment.
> 
> Anyone intending to post feedback in this thread is expected to be familiar with the reviewer guidelines:
> 
> https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md
> 
> *Please stay on topic!*
> 
> Thanks in advance to all who participate.

- I think it is important that the lines marked (1) and (2) in the code below have equivalent validity:

---
struct Iota{
    int s,e;
    void popFront(){ s++; }
    @property bool empty(){ return s>=e; }
    @property int front(){ return s; }
    version(ASSIGNABLE_FRONT) @property void front(int i){ s=i; }
}

void foo(ref int){}

void main(){
    foo(Iota(0,5).front);      // (1)
    foreach(ref x;Iota(0,5)){} // (2)
}
---

As far as I can tell, with the current state of the language, this is what this DIP attempts to propose. Given the precedent of `auto ref` functions, the newly introduced syntax also makes sense, so I am generally in favor of this DIP. (With the understanding that the behavior may or may not change again once we introduce some way to pass rvalues by `ref`.)

- The DIP claims that "It also proposes that current usages of `ref` may be replaced with `auto ref` to retain the current behavior."

This is not correct:

foreach(ref i;iota(0,5)){ // current behavior:
    static assert(__traits(isRef,i));
}
foreach(auto ref i;iota(0,5)){ // new behavior:
    static assert(!__traits(isRef,i));
}


- The DIP fails to address the following cases:

import std.stdio;
void main(){
    foreach(ref x;0..5){ // ???
        if(x==3) x=5;
        writeln(x);
    }
    foreach(auto ref x;0..5){ // ???
        if(x==3) x=5;
        writeln(x);
    }
}

void foo(ref int x){}
void main(){
    [1,2,3][1]=2; // currently: error
    foo([1,2,3][1]); // currently: ok
    foreach(ref x;[1,2,3]){ // ???
        x=2;
    }
    foreach(auto ref x;[1,2,3]){ // ???
        x=2;
    }
}

I think both of these should be discussed explicitly.

On Friday, 25 October 2019 at 12:09:23 UTC, Timon Gehr wrote:
> - I think it is important that the lines marked (1) and (2) in the code below have equivalent validity:
>
> ---
> struct Iota{
>     int s,e;
>     void popFront(){ s++; }
>     @property bool empty(){ return s>=e; }
>     @property int front(){ return s; }
>     version(ASSIGNABLE_FRONT) @property void front(int i){ s=i; }
> }
>
> void foo(ref int){}
>
> void main(){
>     foo(Iota(0,5).front);      // (1)
>     foreach(ref x;Iota(0,5)){} // (2)
> }
> ---
>
> As far as I can tell, with the current state of the language, this is what this DIP attempts to propose.

Yes, exactly.

> - The DIP claims that "It also proposes that current usages of `ref` may be replaced with `auto ref` to retain the current behavior."
>
> This is not correct:
>
> foreach(ref i;iota(0,5)){ // current behavior:
>     static assert(__traits(isRef,i));
> }
> foreach(auto ref i;iota(0,5)){ // new behavior:
>     static assert(!__traits(isRef,i));
> }

Perhaps it's best to just mention that in the "breaking changes". That is probably rare enough to not cause major issues.

>
>
> - The DIP fails to address the following cases:
>
> import std.stdio;
> void main(){
>     foreach(ref x;0..5){ // ???
>         if(x==3) x=5;
>         writeln(x);
>     }
>     foreach(auto ref x;0..5){ // ???
>         if(x==3) x=5;
>         writeln(x);
>     }
> }

Loop A should error, and loop B iterate by copy.

>
> void foo(ref int x){}
> void main(){
>     [1,2,3][1]=2; // currently: error
>     foo([1,2,3][1]); // currently: ok
>     foreach(ref x;[1,2,3]){ // ???
>         x=2;
>     }
>     foreach(auto ref x;[1,2,3]){ // ???
>         x=2;
>     }
> }
>

It could be argued `[1,2,3]` is essentially an enum value, so that `foo([1,2,3][1])` should not compile, and to do otherwise is a bug. In that case, loop A should error. On the other hand, `[1,2,3]` could be seen as a nameless variable in which case loop A should compile.

Either way, loop B must compile like loop A if we allow loop A, otherwise it must iterate by copy.

Any opinions about how loop A should be treated here? I tend to think it should not compile, but I think one could convince me otherwise.

> I think both of these should be discussed explicitly.

For that second one, I probably will. For the first one, see my reply to Walter's criticism - the example will implicitly explain that.

Good points, by the way.