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
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
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
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
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
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
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
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
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
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
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
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
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
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