On Thursday, 8 August 2019 at 13:06:28 UTC, rikki cattermole wrote:
> On 09/08/2019 12:52 AM, Dukc wrote:
>> On Thursday, 8 August 2019 at 12:38:51 UTC, rikki cattermole
>
> 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 :)

Yes, but I can't, quickly at least, come up with any way to reveal those loops without touching the compiler.


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

So it's mainly about minimizing the example more? Fair argument. I'll have to try.

On Thursday, 8 August 2019 at 12:44:23 UTC, Dukc wrote:
> 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.

Like I said in the rest of the paragraph. It restricts it unncecssarily if.you deprecate it. You can't specify the type if you only allow "auto ref". There's no reason to deprecate it. So no, it does not work exactly as ref does now cause it won't work at all in some instances such as when using opApply.

On Thursday, 8 August 2019 at 14:04:30 UTC, Exil wrote:
>
> Like I said in the rest of the paragraph. It restricts it unncecssarily if.you deprecate it. You can't specify the type if you only allow "auto ref". There's no reason to deprecate it. So no, it does not work exactly as ref does now cause it won't work at all in some instances such as when using opApply.

Seems that this one needs clarification. Type should be able to be specified between `auto ref` and the identifier it applies to. Perhaps I need to include a grammar specification.

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!

My sense is that there should be a very simple example up top that makes clear what the problem with the current behavior (simpler than the DynamicArray example later).

On Thursday, 8 August 2019 at 15:13:27 UTC, jmh530 wrote:
>
> My sense is that there should be a very simple example up top that makes clear what the problem with the current behavior (simpler than the DynamicArray example later).

Perhaps. The dynamic array example would be more practical, while the top example would focus on being just dead simple.

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

On Friday, 9 August 2019 at 18:17:50 UTC, Manu wrote:
> [snip]

Wow! It seems that were on totally different pages here! I definitely have to check your link and think long and deeply about your comment before I know how to even begin my response. I'll likely make it tomorrow.

Looks like an opening for an interesting debate, thanks for popping up!

On Fri, Aug 9, 2019 at 2:00 PM Dukc via Digitalmars-d <digitalmars-d@puremagic.com> wrote:
>
> On Friday, 9 August 2019 at 18:17:50 UTC, Manu wrote:
> > [snip]
>
> Wow! It seems that were on totally different pages here! I definitely have to check your link and think long and deeply about your comment before I know how to even begin my response. I'll likely make it tomorrow.
>
> Looks like an opening for an interesting debate, thanks for popping up!

Sorry if that sounded blunt.

Let me try and be more clear; by comparison, we can consider C++,
which has value, ref, and 'auto ref' foreach loops.
All 3 are valid operations, and we have the first 2, but not the 3rd.

C++: for (auto i : things)
D: foreach (i; things)

C++: for (const auto& i : things)
D: foreach (ref i; things) // <- this didn't used to work, but ref
FINALLY accepts rvalues by creating a temporary. I suspect you may be
confusing this case with your auto-ref proposal.

C++: for (auto&& i : things)
D: foreach (auto ref i; things)  // <- you are proposing this, and I
agree, we want this. I've wanted it many times.

I suspect from reading that you are trying to solve the second case
with this DIP, but it's already solved by Andrei's work (which he
presented at dconf).
What this DIP should actually solve though, in the 3rd case. We need that.

On Friday, 9 August 2019 at 21:55:45 UTC, Manu wrote:
>
> Sorry if that sounded blunt.

No, not at all. I was just surprised that you questioned my core resoning, yet wanted the dip to move forward.

>
> Let me try and be more clear; by comparison, we can consider [Snip]
> 
> I suspect from reading that you are trying to solve the second case
> with this DIP, but it's already solved by Andrei's work (which he
> presented at dconf).
> What this DIP should actually solve though, in the 3rd case. We need that.

Hmm, you want to retain the current behaviour of `ref`? This confuses me, because `auto ref` I'm proposing has exactly the same semantics as `ref` does now, yet you also want it to be included.

My guess is that you're thinking the benefit of `auto ref` to be allowing to handle rvalue ranges without becoming a pointer internally, thus being more efficient in at least some cases. The rest of my answer assumes that was your point.

I disagree. I am not trying to target efficiency issues a all. If my problem was that, I'd simply propose foreach `ref` to use rvalue copies directly, not via internal pointers. Unlike in C++, refs and the values they point to are indistinguishable from D user perspective, so that is just an implementation detail that would probably get through even without a DIP. Unless it's already done.

The problem I'm targeting is that currently the compiler will not complain, if the user, when expecting to mutate range elements, iterates by `ref` over foreach. But since I know there's need for the notion I'm deprecating, I also suggest `auto ref` for the behaviour.

I know this is a kind of a jerk change, as it forces to change code. But I thought that the migration path is so simple (just changing `ref` to `auto ref` where you get deprecation messages) that avoiding it (see alternative 1 of the DIP) is not worth having a language inconsistency.

On Saturday, 10 August 2019 at 08:09:08 UTC, Dukc wrote:
> The problem I'm targeting is that currently the compiler will not complain, if the user, when expecting to mutate range elements, iterates by `ref` over foreach.

Meant: iterates by `ref` `foreach` over a range of rvalues