DIP 1022--foreach auto ref--Community Review Round 1

[Manu]

On Thu, Aug 8, 2019 at 5:15 AM Dukc via Digitalmars-d
<digitalmars-d at puremagic.com> 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!

This DIP contains obvious improvement, and should absolutely move
forward, but there are some bugs in this DIP.

"This DIP proposes that foreach loops with the ref keyword applied to
the element variable should only be legal when the elements of the
range have memory addresses."

We create temporaries for ref-args now
(https://www.youtube.com/watch?v=aRvu2JGGn6E), so capturing a ref loop
counter is fine in all cases. But, your DIP is still important, that's
just not the reason.

"It also proposes that current usages ref may be replaced with auto
ref to retain the current behavior."

I'm not sure what you mean by 'replaced', auto ref would simply make
the auto-ref semantic available to foreach loops, which is useful.
'auto ref' in this context only really has anything to do with move
semantics... but I can't see that mentioned anywhere in your DIP.

"This is to ensure that foreach will iterate by reference, while still
allowing iteration over a range of non-copyable elements without
explicit need to adapt the code[1]."

I don't understand this sentence at all... what does it mean?

"...then if one or more elements of aggregate are rvalues[2], a
deprecation message must be emitted including the suggestion to
annotate loopVariable1 with the auto ref keyword instead."

This is wrong, it shouldn't be deprecated; it was only just recently
fixed after a decade of really hard work! (again, see the youtube link
above)

"[...]"

Lots of text... I mean, `auto ref` should be supported in foreach
certainly, but your rationale is all wrong. This DIP is mostly wrong,
even though the thing it wants is correct, the DIP just isn't sure why
it wants it.