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

https://github.com/dlang/DIPs/blob/8f3c24df2acc0f3554875a9decf6314508b72d8a/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 Aug 22, or when I make a post declaring it complete.

At the end of Round 1, 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

I will be moving (copying, deleting, and pasting in a new thread) posts that do not adhere to the reviewer guidelines. So please keep all comments in this thread focused on DIP 1022.

Thanks in advance to all who participate.

On Thursday, 8 August 2019 at 12:10:09 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1022, "foreach auto ref":

Author here, AMA/Destroy!

On Thursday, 8 August 2019 at 12:13:35 UTC, Dukc wrote:
> On Thursday, 8 August 2019 at 12:10:09 UTC, Mike Parker wrote:
>> This is the feedback thread for the first round of Community Review for DIP 1022, "foreach auto ref":
>
> Author here, AMA/Destroy!

Not sure if "foreach( ref" should be deprecated. Doesn't make sense to me with other uses of "auto ref", like in function parameters. They still allow you to use ref. If you did remove it, then I'm not sure how opApply would work in that case. If you want to have both ref and copy, or even just two different types. You can't select the proper ref type because of auto.

On Thursday, 8 August 2019 at 12:13:35 UTC, Dukc wrote:
> On Thursday, 8 August 2019 at 12:10:09 UTC, Mike Parker wrote:
>> This is the feedback thread for the first round of Community Review for DIP 1022, "foreach auto ref":
>
> Author here, AMA/Destroy!

Just a note: the patch I submitted aimed to fix a "lifecycle" problem, I wasn't trying to fix the behaviour. foreach+ref with rvalues was creating a copy of each element without calling its dtor.

Good work, I hope this dip will be approved!

- In the first alternative a disadvantage is listed but no advantages. An advantage of it is that there would be no breaking changes unlike the proposed solution which has an unknown amount of breakage.

If we could get an idea of the severity (1-2 modules vs half vs almost all of Phobos) of the breakage being proposed, that would great.

- What is the interaction with indexes? No mention of them, nor example.

- The examples you have may look good for an implementer, but for everybody else perhaps some examples using concrete types that ignore the foreach body would be a good edition. Just to make it clear what we're getting out of it. I.e.

RangeWithoutRefFront expr; // T front()

foreach(v; expr) // ok
foreach(ref v; expr) // Success but will be error after deprecation
foreach(auto ref v; expr) // ok

On Thursday, 8 August 2019 at 12:36:07 UTC, Andrea Fontana wrote:
>
> Just a note: the patch I submitted aimed to fix a "lifecycle" problem, I wasn't trying to fix the behaviour. foreach+ref with rvalues was creating a copy of each element without calling its dtor.
>
> Good work, I hope this dip will be approved!

Um, I understood that this is what you PR finally did, but didn't you intend to disable foreach-ref with rvalues at some point (when Andrei made the comment I mentioned)?

On Thursday, 8 August 2019 at 12:35:45 UTC, Exil wrote:
>
> Not sure if "foreach( ref" should be deprecated.

I'm not quite sure what you meant, but if you're afraid that the DIP will deprecate foreach-ref in whole, no. It will only deprecate foreach-ref with rvalues. Auto ref will work exactly as ref does now.

On Thursday, 8 August 2019 at 12:38:51 UTC, rikki cattermole wrote:
> - In the first alternative a disadvantage is listed but no advantages. An advantage of it is that there would be no breaking changes unlike the proposed solution which has an unknown amount of breakage.

Read the paragraph slower :D.

>
> If we could get an idea of the severity (1-2 modules vs half vs almost all of Phobos) of the breakage being proposed, that would great.

Remember that nothing will break before deprecation period ends. If it proves to be too bad we can back up and go with alternative #1 instead.

>
> - What is the interaction with indexes? No mention of them, nor example.

Yes, this is likely worth an explicit mention. Same as with the equivalent regular or ref foreach, depending on which is chosen according to the rules menitoned on the DIP.

>
> - The examples you have may look good for an implementer, but for everybody else perhaps some examples using concrete types that ignore the foreach body would be a good edition. Just to make it clear what we're getting out of it. I.e.
>
> RangeWithoutRefFront expr; // T front()
>
> foreach(v; expr) // ok
> foreach(ref v; expr) // Success but will be error after deprecation
> foreach(auto ref v; expr) // ok

Isn't the EMSI-containers example already like yours? Or did you mean that your example as how-not-to example?

On 09/08/2019 12:52 AM, Dukc wrote:
> On Thursday, 8 August 2019 at 12:38:51 UTC, rikki cattermole wrote:
>> - In the first alternative a disadvantage is listed but no advantages. An advantage of it is that there would be no breaking changes unlike the proposed solution which has an unknown amount of breakage.
> 
> Read the paragraph slower :D.

Darn it, you're right!

>>
>> If we could get an idea of the severity (1-2 modules vs half vs almost all of Phobos) of the breakage being proposed, that would great.
> 
> Remember that nothing will break before deprecation period ends. If it proves to be too bad we can back up and go with alternative #1 instead.

If we can do a little bit of research now to prevent changing an implementation after a DIP is accepted, we probably should.

When no code is written, changing tactic is cheap :)

>>
>> - What is the interaction with indexes? No mention of them, nor example.
> 
> Yes, this is likely worth an explicit mention. Same as with the equivalent regular or ref foreach, depending on which is chosen according to the rules menitoned on the DIP.
> 
>>
>> - The examples you have may look good for an implementer, but for everybody else perhaps some examples using concrete types that ignore the foreach body would be a good edition. Just to make it clear what we're getting out of it. I.e.
>>
>> RangeWithoutRefFront expr; // T front()
>>
>> foreach(v; expr) // ok
>> foreach(ref v; expr) // Success but will be error after deprecation
>> foreach(auto ref v; expr) // ok
> 
> Isn't the EMSI-containers example already like yours? Or did you mean that your example as how-not-to example?

The EMSI-containers example while useful, has a lot of extra code that you have to think about. Or at least it does for me.

Basically what I would like to have is an expression +
foreach statements that are current (and will remain) +
foreach statements that will no longer be valid +
foreach statements that will become valid

Essentially the bare bones of what is changing and something to compare against. So if the only thing you look at within the DIP, this will tell you pretty much what you need to know.

On Thursday, 8 August 2019 at 12:52:43 UTC, Dukc wrote:
> On Thursday, 8 August 2019 at 12:38:51 UTC, rikki cattermole wrote:
>>
>> If we could get an idea of the severity (1-2 modules vs half vs almost all of Phobos) of the breakage being proposed, that would great.
>
> Remember that nothing will break before deprecation period ends. If it proves to be too bad we can back up and go with alternative #1 instead.
>

Just realized how stupid this would be in case of Phobos, if just about standard library usage will spit deprecation messages all over the place.

Phobos has to be migrated with a compiler fork that has this feature implemented (running it against the standard test suite should reveal a good enough portion of offending loops), and if it proves to be too bad to migrate them behind the scenes, then a preview switch.

Sounds unlikely to me that a switch would be needed, since it's just about changing `ref`s to `auto ref`s. So probably not worth it to add anything extra mentions of that to the DIP. But the main contributors are better at estimating that than me -any opinions?