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

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.

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.

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?

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

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

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?

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.

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.