www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - DIP 1022---foreach auto ref---Community Review Round 2

reply Mike Parker <aldacron gmail.com> writes:
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.
Oct 19 2019
next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
I find the Rationale section to be completely confusing as to:

1. what the existing state of the compiler is
2. what the proposed change is

For example,

"A pull request[3] was submitted to disallow this behavior."

That PR was merged, although the Rationale implies it was not.

Further, that PR appears to disallow ref being applied to temporaries, yet the 
prose goes on to imply it does:

"If a range with non-reference semantics is accidently passed to such a loop,
it 
will be iterated by value, which is likely to be unexpected."

The "A simple example" is not simple at all, because it imports std.range:iota 
and is not possible to understand without examining the source to iota.
Oct 20 2019
parent reply Dukc <ajieskola gmail.com> writes:
On Sunday, 20 October 2019 at 08:32:57 UTC, Walter Bright wrote:

 I find the Rationale section to be completely confusing as to:

 1. what the existing state of the compiler is
 2. what the proposed change is


What do you suggest? Mike said that it's standard procedure to 
put rationale first and description of the change second. How can 
I be more explicit?


 For example,

 "A pull request[3] was submitted to disallow this behavior."

 That PR was merged, although the Rationale implies it was not.


The rationale says that it did not end up disabling `foreach` by 
`ref` over rvalue ranges - the PR was merged, but the final 
change was not what it was when Andrei commented. Byt yea, I 
should have been more explicit.


 Further, that PR appears to disallow ref being applied to 
 temporaries, yet the prose goes on to imply it does:

 "If a range with non-reference semantics is accidently passed 
 to such a loop, it will be iterated by value, which is likely 
 to be unexpected."


That phrase means the current situation, not the one after the 
DIP. Is there something that implies otherwise?


 The "A simple example" is not simple at all, because it imports 
 std.range:iota and is not possible to understand without 
 examining the source to iota.


Now when I think of it, I can as well make the `foreach`s iterate 
over `0 .. 5` instead. Didn't occur to me when I made that 
example.
Oct 20 2019
parent reply Mike Parker <aldacron gmail.com> writes:
On Sunday, 20 October 2019 at 16:47:47 UTC, Dukc wrote:

 On Sunday, 20 October 2019 at 08:32:57 UTC, Walter Bright wrote:

 I find the Rationale section to be completely confusing as to:

 1. what the existing state of the compiler is
 2. what the proposed change is


 What do you suggest? Mike said that it's standard procedure to 
 put rationale first and description of the change second. How 
 can I be more explicit?


Yes, the Rationale section comes before the Description section. 
That's not what he's referring to. He's asking that the current 
rationale be revised for clarity. We can work on that when this 
review round is complete.
Oct 22 2019
next sibling parent Dukc <ajieskola gmail.com> writes:
On Wednesday, 23 October 2019 at 03:55:22 UTC, Mike Parker wrote:

 That's not what he's referring to. He's asking that the current 
 rationale be revised for clarity. We can work on that when this 
 review round is complete.


Yes, but can you tell here any ideas how can I clarify the 
rationale?
Oct 23 2019
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 10/22/2019 8:55 PM, Mike Parker wrote:

 On Sunday, 20 October 2019 at 16:47:47 UTC, Dukc wrote:

 On Sunday, 20 October 2019 at 08:32:57 UTC, Walter Bright wrote:

 I find the Rationale section to be completely confusing as to:

 1. what the existing state of the compiler is
 2. what the proposed change is


 What do you suggest? Mike said that it's standard procedure to put rationale 
 first and description of the change second. How can I be more explicit?


 
 Yes, the Rationale section comes before the Description section. That's not
what 
 he's referring to. He's asking that the current rationale be revised for 
 clarity. We can work on that when this review round is complete.


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.
Oct 23 2019
parent Dukc <ajieskola gmail.com> writes:
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.
Oct 23 2019
prev sibling next sibling parent reply Dukc <ajieskola gmail.com> writes:
On Saturday, 19 October 2019 at 13:16:17 UTC, Mike Parker wrote:

 This is the feedback thread for the second round of Community 
 Review for DIP 1022, "foreach auto ref":


In case someone does not have concentration to see what's changed 
relative to the first community review, the major things are:

-`static foreach` is no longer supposed to compile with `auto 
ref`. Rationale of why in description.

-added a simplified example in bottom of rationale section
Oct 23 2019
parent reply Andrea Fontana <nospam example.com> writes:
On Wednesday, 23 October 2019 at 07:25:40 UTC, Dukc wrote:

 On Saturday, 19 October 2019 at 13:16:17 UTC, Mike Parker wrote:

 This is the feedback thread for the second round of Community 
 Review for DIP 1022, "foreach auto ref":


 In case someone does not have concentration to see what's 
 changed relative to the first community review, the major 
 things are:

 -`static foreach` is no longer supposed to compile with `auto 
 ref`. Rationale of why in description.

 -added a simplified example in bottom of rationale section


I'm the author of that pull request. Reading your DIP I think you 
give a misinterpretation of it.

"A pull request[3] was submitted to disallow this behavior.

In every place where foreach (ref <...>) presently iterates by 
value, a compiler error would have resulted after merging that 
pull request."

My patch was not supposed to give any error. And I didn't even 
try to disallow this behaviour.

Before that patch, compiler allowed to iterate a range of structs 
(where front is returned by value) by ref, that probably doesn't 
make any sense. In this case dtor on those structs is never 
called due to a bug in code. This mean that if my struct wrap a 
"malloc" call in struct's ctor and a "free" call in struct's dtor 
(or a refcount like in my case) memory is saturated using a 
foreach (or simply usign phobos "each!T").

My patch simply fixed that missing dtor call, and I left the 
behaviour change for a complex DIP.

Sönke Ludwig summarized [1] this better than me in a comment on 
github:
"Isn't this quite simple? Fixing the failure to call the 
destructor is critical and a clear improvement over the status 
quo. The ref foreach didn't ever do something useful for r-value 
ranges, but that's an orthogonal issue that should simply be 
discussed in the context of a separate ticket (or DIP)."


[1] https://github.com/dlang/dmd/pull/8437#issuecomment-466249392
Oct 23 2019
parent reply Dukc <ajieskola gmail.com> writes:
On Wednesday, 23 October 2019 at 08:41:54 UTC, Andrea Fontana 
wrote:

 I'm the author of that pull request. Reading your DIP I think 
 you give a misinterpretation of it.


Yeah, I obviously either need to be more explicit about your PR 
or remove that citation. But if I recall correctly, your PR tried 
at some point to disallow rvalue ref foreachs. I don't mean the 
version that was merged, but the variant that was at the reviews 
when Andrei made the comment I cited. Is this correct?
Oct 23 2019
parent reply Andrea Fontana <nospam example.com> writes:
On Wednesday, 23 October 2019 at 08:57:56 UTC, Dukc wrote:

 On Wednesday, 23 October 2019 at 08:41:54 UTC, Andrea Fontana 
 wrote:

 I'm the author of that pull request. Reading your DIP I think 
 you give a misinterpretation of it.


 Yeah, I obviously either need to be more explicit about your PR 
 or remove that citation. But if I recall correctly, your PR 
 tried at some point to disallow rvalue ref foreachs. I don't 
 mean the version that was merged, but the variant that was at 
 the reviews when Andrei made the comment I cited. Is this 
 correct?


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.
Oct 23 2019
parent Dukc <ajieskola gmail.com> writes:
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.
Oct 23 2019
prev sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
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/089816bc47ee3d1df06d10aa2af943d3e70e
161/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.
Oct 25 2019
parent Dukc <ajieskola gmail.com> writes:
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.
Oct 26 2019