www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - DIP 1022--foreach auto ref--Community Review Round 1

reply Mike Parker <aldacron gmail.com> writes:
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.
Aug 08
next sibling parent reply Dukc <ajieskola gmail.com> writes:
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!
Aug 08
next sibling parent reply Exil <Exil gmall.com> writes:
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.
Aug 08
parent reply Dukc <ajieskola gmail.com> writes:
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.
Aug 08
parent reply Exil <Exil gmall.com> writes:
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.
Aug 08
parent Dukc <ajieskola gmail.com> writes:
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.
Aug 08
prev sibling next sibling parent reply Andrea Fontana <nospam example.com> writes:
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!
Aug 08
parent Dukc <ajieskola gmail.com> writes:
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)?
Aug 08
prev sibling next sibling parent reply jmh530 <john.michael.hall gmail.com> writes:
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).
Aug 08
parent Dukc <ajieskola gmail.com> writes:
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.
Aug 08
prev sibling parent reply Manu <turkeyman gmail.com> writes:
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.
Aug 09
parent reply Dukc <ajieskola gmail.com> writes:
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!
Aug 09
parent reply Manu <turkeyman gmail.com> writes:
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.
Aug 09
parent reply Dukc <ajieskola gmail.com> writes:
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.
Aug 10
next sibling parent Dukc <ajieskola gmail.com> writes:
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
Aug 10
prev sibling next sibling parent Dukc <ajieskola gmail.com> writes:
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
Aug 10
prev sibling parent reply Manu <turkeyman gmail.com> writes:
On Sat, Aug 10, 2019 at 1:11 AM Dukc via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 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.
I'm thought about it some more, and I think you're right about the deprecation. I think I can agree that the probability of confusion + mistake where a ref to temporary exists is higher weighted than the uniformity with function calling. I tend to hate special-case rules (in this case, loop counters behaving differently than function arguments), but I think I'm persuaded here. I support your text unamended ;) .. we've needed `auto ref` in foreach for a long time. I'm curious about this line: "It should be allowed in static foreach, but with no effect, as elements of compile-time aggregates can never be lvalues." Is that true? I don't think there's any reason they can't be lvalues. But what I'm trying to get my head around is if the loop counter for `static foreach` is a variable at all, or if it's an alias? I think there's inherent complexity here, because there's a lot of cases... and some should work with auto ref, but not all. Okay, here's some experiment cases: int x = 1; static foreach (i; AliasSeq!(10, x, 20)) pragma(msg, __traits(isRef, i));
 false false false
In this case, `i` is an alias. Ref-ness doesn't mean anything here. This can't work: int x = 1; static foreach (int i; AliasSeq!(10, x, 20)) pragma(msg, __traits(isRef, i));
 Error: can't initialise `i` because `x` is a runtime value
Because it tries to initialise an int from elements of tuple, and `x` is a runtime variable. This could work if ref was treated the same way as parameter arguments: int x = 1; static foreach (ref int i; AliasSeq!(10, x, 20)) pragma(msg, __traits(isRef, i));
 true true true
In that case, rvalues (which can be determined at compile time) would populate temporaries, the same way as function parameters. But according to the rules in your dip, we don't allow that, and I conceded above. This should work though: int x = 1; static foreach (ref int i; AliasSeq!(x, x, x)) pragma(msg, __traits(isRef, i));
 true true true
All elements are lvalues, so we accept `ref`. Finally, this should work as expected: int x = 1; static foreach (auto ref int i; AliasSeq!(10, x, 20)) pragma(msg, __traits(isRef, i));
 false true false
Functionally identical to the first case, except the interesting distinction that `i` contains information which can be used in introspection logic, which is almost always present in static foreach bodies. I believe this should be allowed AND behave as expected; that is; `i` is a ref when encountering an lvalue. If it does not behave in this way, it should not be allowed. Now, there's a different set of cases, where you call static foreach over a not-a-tuple: static foreach (i; [10, 20, 30]) pragma(msg, __traits(isRef, i));
 false false false
In this case, `i` is not an alias like the previous experiment, it is inferred to be `int`. Should be the same as `static foreach (int i; ...)`, right? static foreach (ref i; [10, 20, 30]) pragma(msg, __traits(isRef, i));
 true true true
As above, we could allow this by creating temporaries the same as function arguments... but we've decided not to allow ref iterators from rvalues. static foreach (auto ref i; [10, 20, 30]) pragma(msg, __traits(isRef, i));
 false false false ??
I guess the whole array is an rvalue, so then the loop counter would take copies of the elements? It gets interesting when you do this: int[3] x = [10, 20, 30]; static foreach (ref i; x) pragma(msg, __traits(isRef, i));
 true true true
And: int[3] x = [10, 20, 30]; static foreach (auto ref i; x) pragma(msg, __traits(isRef, i));
 true true true
These should work as expected. TL;DR, your sentence "It should be allowed in static foreach, but with no effect" should be removed, and if you want to detail the expected semantics with `static foreach`, that might be a good idea.
Aug 10
next sibling parent reply Nick Treleaven <nick geany.org> writes:
On Saturday, 10 August 2019 at 21:42:00 UTC, Manu wrote:
   static foreach (ref i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 true true true
As above, we could allow this by creating temporaries the same as function arguments... but we've decided not to allow ref iterators from rvalues.
It doesn't need temporaries, [2][0] is an lvalue with existing dmd: void f(ref int i); void main(){ f([2][0]); //compiles }
 TL;DR, your sentence "It should be allowed in static foreach, 
 but with no effect" should be removed, and if you want to 
 detail the expected semantics with `static foreach`, that might 
 be a good idea.
+1
Aug 11
next sibling parent Manu <turkeyman gmail.com> writes:
On Sun, Aug 11, 2019 at 7:55 AM Nick Treleaven via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On Saturday, 10 August 2019 at 21:42:00 UTC, Manu wrote:
   static foreach (ref i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 true true true
As above, we could allow this by creating temporaries the same as function arguments... but we've decided not to allow ref iterators from rvalues.
It doesn't need temporaries, [2][0] is an lvalue with existing dmd: void f(ref int i); void main(){ f([2][0]); //compiles }
Oh yeah... I forgot about that. Caused me so much trouble over the years. So bad!
 TL;DR, your sentence "It should be allowed in static foreach,
 but with no effect" should be removed, and if you want to
 detail the expected semantics with `static foreach`, that might
 be a good idea.
+1
Aug 11
prev sibling parent Dukc <ajieskola gmail.com> writes:
On Sunday, 11 August 2019 at 14:52:08 UTC, Nick Treleaven wrote:
 void f(ref int i);
 void main(){
     f([2][0]); //compiles
 }
Huh? What happens here?
Aug 13
prev sibling parent reply Dukc <ajieskola gmail.com> writes:
On Saturday, 10 August 2019 at 21:42:00 UTC, Manu wrote:

 I tend to hate special-case rules (in this case, loop counters
 behaving differently than function arguments), but I think I'm
 persuaded here.
Not sure what you mean here... example?
 Okay, here's some experiment cases:

   int x = 1;
   static foreach (i; AliasSeq!(10, x, 20))
       pragma(msg, __traits(isRef, i));

 false false false
In this case, `i` is an alias. Ref-ness doesn't mean anything here.
I thought that would compile only without the `static` keyword. Gotta investigate. If it compiles, I'm going to say it should behave exactly as written without the `static` keyword. Same answer for the rest of examples with tuples.
 Now, there's a different set of cases, where you call static 
 foreach over a not-a-tuple:

   static foreach (i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 false false false
In this case, `i` is not an alias like the previous experiment, it is inferred to be `int`. Should be the same as `static foreach (int i; ...)`, right? static foreach (ref i; [10, 20, 30]) pragma(msg, __traits(isRef, i));
 true true true
Yes, these are the cases I meant with `static foreach`. I think `[10, 20, 30]` is a rvalue, and if the latter of these two examples compiles, it should not.
 As above, we could allow this by creating temporaries the same 
 as function arguments... but we've decided not to allow ref 
 iterators from rvalues.

   static foreach (auto ref i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 false false false ??
Yes, exactly what's supposed to happen.
 I guess the whole array is an rvalue, so then the loop counter 
 would take copies of the elements?

 It gets interesting when you do this:

   int[3] x = [10, 20, 30];
   static foreach (ref i; x)
       pragma(msg, __traits(isRef, i));

 true true true
I don't think that compiles. x is not an enum value. But I might be wrong, I'll have to recheck this also.
 And:

   int[3] x = [10, 20, 30];
   static foreach (auto ref i; x)
       pragma(msg, __traits(isRef, i));

 true true true
Same as above. If x was enum, then it should not compile with `ref` and should return false false false with `auto ref`.
 TL;DR, your sentence "It should be allowed in static foreach, 
 but with no effect" should be removed, and if you want to 
 detail the expected semantics with `static foreach`, that might 
 be a good idea.
I will check those examples that I thought won't compile do. If they do, I'll be more explicit.
Aug 13
parent reply Manu <turkeyman gmail.com> writes:
On Tue, Aug 13, 2019 at 6:50 AM Dukc via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On Saturday, 10 August 2019 at 21:42:00 UTC, Manu wrote:

 I tend to hate special-case rules (in this case, loop counters
 behaving differently than function arguments), but I think I'm
 persuaded here.
Not sure what you mean here... example?
struct S { int front(); ... } void takesRef(ref int); { S s; takesRef(s.front()); // <- fine, temporary is created and passed by ref foreach(ref i; s) // <- error: no temporary is created; we have decided to emit an error where an rval is given to a loop counter { ... } } This is the special case I'm talking about.
 Okay, here's some experiment cases:

   int x = 1;
   static foreach (i; AliasSeq!(10, x, 20))
       pragma(msg, __traits(isRef, i));

 false false false
In this case, `i` is an alias. Ref-ness doesn't mean anything here.
I thought that would compile only without the `static` keyword. Gotta investigate. If it compiles, I'm going to say it should behave exactly as written without the `static` keyword.
Iterating a tuple is a compile-time expansion, which is distinct from a runtime expansion. The result is `false false false` either way, but static and non-static are very different operations. `foreach` will iterate an array of int's, `static foreach` will expand the tuple and `i` will be an alias of each element.
 Same answer for the rest of examples with tuples.
No, static and non-static foreach are completely different semantically. My examples may show the same outputs either way, but it's easy to make cases which show the distinction between static and non-static.
 Now, there's a different set of cases, where you call static
 foreach over a not-a-tuple:

   static foreach (i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 false false false
In this case, `i` is not an alias like the previous experiment, it is inferred to be `int`. Should be the same as `static foreach (int i; ...)`, right? static foreach (ref i; [10, 20, 30]) pragma(msg, __traits(isRef, i));
 true true true
Yes, these are the cases I meant with `static foreach`. I think `[10, 20, 30]` is a rvalue, and if the latter of these two examples compiles, it should not.
Actually, because of D's 'weird shit' law, the array is not an rvalue. (I think...?)
 As above, we could allow this by creating temporaries the same
 as function arguments... but we've decided not to allow ref
 iterators from rvalues.

   static foreach (auto ref i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 false false false ??
Yes, exactly what's supposed to happen.
Right... what's your point? Incidentally, I'm not sure this will hold; because array literals are not rvalues, so I think it might be `true true true` here...
 I guess the whole array is an rvalue, so then the loop counter
 would take copies of the elements?

 It gets interesting when you do this:

   int[3] x = [10, 20, 30];
   static foreach (ref i; x)
       pragma(msg, __traits(isRef, i));

 true true true
I don't think that compiles. x is not an enum value. But I might be wrong, I'll have to recheck this also.
Why not? It's an alias... it should compile just fine.
 And:

   int[3] x = [10, 20, 30];
   static foreach (auto ref i; x)
       pragma(msg, __traits(isRef, i));

 true true true
Same as above. If x was enum, then it should not compile with `ref` and should return false false false with `auto ref`.
`x` is not an enum, it's a local. The cases I propose to consider are exactly what I wrote, not some other thing. For instance: int fun(ref int x) { static foreach (i; AliasSeq!(10, x, 30)) // <- outputs `false true false` pragma(msg, __traits(isRef, i)); foreach (i; AliasSeq!(10, x, 30)) // <- outputs `false false false` (!!!) pragma(msg, __traits(isRef, i)); } And there's another interesting case of 'weird shit' here too. That second one should output `false`, not `false false false`; the pragma(msg) should only evaluate once, because the non-static foreach shouldn't be expanding... but strangely, runtime foreach when given a tuple behaves almost like a static foreach, but the loop counter is not an alias for the tuple element, it is rather an `auto i = element[n]` copy. I actually think this form of runtime foreach should be deprecated, if you want to do this with runtime foreach, wrap the tuple in `[ ]`. Here there be dargons!
 TL;DR, your sentence "It should be allowed in static foreach,
 but with no effect" should be removed, and if you want to
 detail the expected semantics with `static foreach`, that might
 be a good idea.
I will check those examples that I thought won't compile do. If they do, I'll be more explicit.
I'm not strictly talking about what does actually compile, I'm talking about what makes semantic sense, and would be uniform semantically with no bizarre cases. Some cases might not work, but should.
Aug 13
next sibling parent reply Dukc <ajieskola gmail.com> writes:
On Tuesday, 13 August 2019 at 23:34:32 UTC, Manu wrote:
 struct S
 {
   int front();
   ...
 }

 void takesRef(ref int);

 {
   S s;

   takesRef(s.front()); // <- fine, temporary is created and 
 passed by ref

   foreach(ref i; s) // <- error: no temporary is created; we 
 have
 decided to emit an error where an rval is given to a loop 
 counter
   { ... }
 }

 This is the special case I'm talking about.
Nope, does not pass: `Error: function onlineapp.takesRef(ref int integer) is not callable using argument types (int)`
 Okay, here's some experiment cases:

   int x = 1;
   static foreach (i; AliasSeq!(10, x, 20))
       pragma(msg, __traits(isRef, i));

 false false false
Iterating a tuple is a compile-time expansion, which is distinct from a runtime expansion. The result is `false false false` either way, but static and non-static are very different operations. `foreach` will iterate an array of int's, `static foreach` will expand the tuple and `i` will be an alias of each element.
No. `foreach` over an alias sequence will be unrolled at compile time, and behave like a `static foreach` does with CTFEd arrays. That was the way we did what `static foreach` does now, remember? But I have to confess I'm not sure what `static foreach` does over an alias sequence. I ran a few tests, and some compiled despite my expectations, but didn't get a clear picture what it does. I think I need to read `static foreach` DIP again.
 Same answer for the rest of examples with tuples.
No, static and non-static foreach are completely different semantically. My examples may show the same outputs either way, but it's easy to make cases which show the distinction between static and non-static.
See above.
 Yes, these are the cases I meant with `static foreach`. I 
 think `[10, 20, 30]` is a rvalue, and if the latter of these 
 two examples compiles, it should not.
Actually, because of D's 'weird shit' law, the array is not an rvalue. (I think...?)
It is. I tested that: `Error: constant value 10 cannot be ref`
 As above, we could allow this by creating temporaries the 
 same as function arguments... but we've decided not to allow 
 ref iterators from rvalues.

   static foreach (auto ref i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 false false false ??
Yes, exactly what's supposed to happen.
Right... what's your point? Incidentally, I'm not sure this will hold; because array literals are not rvalues, so I think it might be `true true true` here...
Because the loop won't work with `ref`, it falls back to iteration-by-copy. But you're right that if the literals were not rvalues, that would result in `true true true`.
 I guess the whole array is an rvalue, so then the loop 
 counter would take copies of the elements?

 It gets interesting when you do this:

   int[3] x = [10, 20, 30];
   static foreach (ref i; x)
       pragma(msg, __traits(isRef, i));

 true true true
Why not? It's an alias... it should compile just fine.
x is a runtime static array. Tested that also: `Error: variable x cannot be read at compile time`
 And:

   int[3] x = [10, 20, 30];
   static foreach (auto ref i; x)
       pragma(msg, __traits(isRef, i));
`x` is not an enum, it's a local. The cases I propose to consider are exactly what I wrote, not some other thing.
You are right it is not an enum, like I also just said. But that's exactly the reason why it cannot compile, `ref` (or `auto ref`) or no. `static foreach` requires the values of the loop aggregate to be available at compile time, not just it's length. I might well support an argument that this shouldn't be the case, but as it is, my DIP should not need to discuss that case.
 For instance:

 int fun(ref int x)
 {
   static foreach (i; AliasSeq!(10, x, 30))  // <- outputs 
 `false true false`
     pragma(msg, __traits(isRef, i));

   foreach (i; AliasSeq!(10, x, 30)) // <- outputs `false false 
 false` (!!!)
     pragma(msg, __traits(isRef, i));
 }

 [snip]
 I actually think this form of runtime foreach should be 
 deprecated, if
 you want to do this with runtime foreach, wrap the tuple in `[ 
 ]`.
An interesting idea for another potential DIP.
 I'm not strictly talking about what does actually compile, I'm 
 talking about what makes semantic sense, and would be uniform 
 semantically with no bizarre cases. Some cases might not work, 
 but should.
Perhaps, in some cases. But again, it's outside the scope of this DIP.
Aug 14
parent reply Manu <turkeyman gmail.com> writes:
On Wed, Aug 14, 2019 at 8:40 AM Dukc via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On Tuesday, 13 August 2019 at 23:34:32 UTC, Manu wrote:
 struct S
 {
   int front();
   ...
 }

 void takesRef(ref int);

 {
   S s;

   takesRef(s.front()); // <- fine, temporary is created and
 passed by ref

   foreach(ref i; s) // <- error: no temporary is created; we
 have
 decided to emit an error where an rval is given to a loop
 counter
   { ... }
 }

 This is the special case I'm talking about.
Nope, does not pass: `Error: function onlineapp.takesRef(ref int integer) is not callable using argument types (int)`
You need -preview=rvalueRefParam I pointed you to Andrei's lecture that talks about this at the start of the thread.
 Okay, here's some experiment cases:

   int x = 1;
   static foreach (i; AliasSeq!(10, x, 20))
       pragma(msg, __traits(isRef, i));

 false false false
Iterating a tuple is a compile-time expansion, which is distinct from a runtime expansion. The result is `false false false` either way, but static and non-static are very different operations. `foreach` will iterate an array of int's, `static foreach` will expand the tuple and `i` will be an alias of each element.
No. `foreach` over an alias sequence will be unrolled at compile time, and behave like a `static foreach` does with CTFEd arrays. That was the way we did what `static foreach` does now, remember?
Yes, I recall. But static foreach exists now, so I feel like it should be deprecated in favour of static foreach.
 But I have to confess I'm not sure what `static foreach` does
 over an alias sequence. I ran a few tests, and some compiled
 despite my expectations, but didn't get a clear picture what it
 does. I think I need to read `static foreach` DIP again.
It does this: alias i = tuple[0]; ... body alias i = tuple[1]; ... body alias i = tuple[2]; ... body This is why the ref function arg shows ref correctly. It unrolls the loop for each element of the tuple as an alias.
 Same answer for the rest of examples with tuples.
No, static and non-static foreach are completely different semantically. My examples may show the same outputs either way, but it's easy to make cases which show the distinction between static and non-static.
See above.
?
 Yes, these are the cases I meant with `static foreach`. I
 think `[10, 20, 30]` is a rvalue, and if the latter of these
 two examples compiles, it should not.
Actually, because of D's 'weird shit' law, the array is not an rvalue. (I think...?)
It is. I tested that: `Error: constant value 10 cannot be ref`
Okay, good. There was a time where array literals use to allocate using the GC...
 As above, we could allow this by creating temporaries the
 same as function arguments... but we've decided not to allow
 ref iterators from rvalues.

   static foreach (auto ref i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 false false false ??
Yes, exactly what's supposed to happen.
Right... what's your point? Incidentally, I'm not sure this will hold; because array literals are not rvalues, so I think it might be `true true true` here...
Because the loop won't work with `ref`, it falls back to iteration-by-copy. But you're right that if the literals were not rvalues, that would result in `true true true`.
Right, sorry, I wasn't sure the current state of array literals. There was a time when they allocated memory.
 I guess the whole array is an rvalue, so then the loop
 counter would take copies of the elements?

 It gets interesting when you do this:

   int[3] x = [10, 20, 30];
   static foreach (ref i; x)
       pragma(msg, __traits(isRef, i));

 true true true
Why not? It's an alias... it should compile just fine.
x is a runtime static array. Tested that also: `Error: variable x cannot be read at compile time`
It's not being read, just referenced. This can/should work, it just doesn't. It should effectively be the same as: int[3] x; static foreach (i; AliasSeq!(x[0], x[1], x[2])) {} Which doesn't work, but this does: int x, y, z; static foreach (i; AliasSeq!(x, y, z)) {} This also works: int[3] x; static foreach (i; 0 .. 3) { ... x[i] ... } Unrolling an array is super useful, we just don't support it... but we could!
 And:

   int[3] x = [10, 20, 30];
   static foreach (auto ref i; x)
       pragma(msg, __traits(isRef, i));
`x` is not an enum, it's a local. The cases I propose to consider are exactly what I wrote, not some other thing.
You are right it is not an enum, like I also just said. But that's exactly the reason why it cannot compile, `ref` (or `auto ref`) or no. `static foreach` requires the values of the loop aggregate to be available at compile time, not just it's length.
x is right there, or the code couldn't compile. Our meta is perfectly fine with symbol aliases. For the non-ref (ie, alias) case, sub the array expansion with `(x, y, z)` instead of `(x[0], x[1], x[2])`. It's exactly the same thing, it's just not supported is all.
From there, whether we support static foreach fabricating a ref that
points to an alias is a different question, but it's totally possible. It's not any different for the compiler than this: void fun(ref int); int x; static foreach(ref i; AliasSeq!(x, x, x)) {} // <- i is a reference to a local fun(x); fun(x); fun(x); // <- function receives a reference to local It makes ref to x in both cases.
 I might well support an argument that this shouldn't be the case,
 but as it is, my DIP should not need to discuss that case.
It's just that you specifically mention static foreach and that it's meaningless; which I'm not sure is true. You should either not mention static foreach (your DIP applies to runtime foreach), or should say what static foreach should support, and that may be "static foreach does not interact with ref", but it's possible that it could, and could be useful in the future. I think there's just a few gaps still where things that should work haven't been requested. Loop unrolling primarily. We can't alias expressions at all.
 I'm not strictly talking about what does actually compile, I'm
 talking about what makes semantic sense, and would be uniform
 semantically with no bizarre cases. Some cases might not work,
 but should.
Perhaps, in some cases. But again, it's outside the scope of this DIP.
I was just pointing out that I think this text should change: "It should be allowed in static foreach, but with no effect, as elements of compile-time aggregates can never be lvalues." Elements CAN be lvalues in static foreach, that's what I was trying to show: int x, y, z; static foreach (i; AliasSeq!(x, y, z)) {} // <- compiles in today's language, all elements are lvalues. I'm saying, if you allow `auto ref` in static foreach, it should do the thing (capture lvalues by ref). I think you should change it to say it is not allowed in static foreach, at least until we work out if/how ref in static foreach should work. I think it should work naturally; ref's to symbol aliases is possible, and I think the uniformity would be sensible, and I agree it's outside this DIP, which is why you should change that text.
Aug 14
parent reply Dukc <ajieskola gmail.com> writes:
On Wednesday, 14 August 2019 at 18:26:21 UTC, Manu wrote:
 You need -preview=rvalueRefParam
 I pointed you to Andrei's lecture that talks about this at the 
 start
 of the thread.
Now THAT was news to me! I had flipped through the slides of that lecture some day but I didn't remember we had such a review switch. I definitely need to address that somehow. To be frank, I may have to recommend considering alternative #1 instead of the DIP if that switch is planned to be mainstream.
 It does this:

 alias i = tuple[0];
 ... body
 alias i = tuple[1];
 ... body
 alias i = tuple[2];
 ... body

 This is why the ref function arg shows ref correctly. It 
 unrolls the loop for each element of the tuple as an alias.
Yeah, but for some reason a plain `foreach` will compile over an `AliasSeq` with ref (if all elements are lvalues), but `static foreach` won't (according to my tests). Might be a bug, the error message seemed strange. I suppose that `foreach` and `static foreach` are intended behave exactly the same in this case. Before considering your proposal of deprecating non-static `foreach` for this use, that is. I am going to mention that `static foreach` over alias sequence behaves exactly as without the keyword. I'll also make clear that "no rvalues possible for `static foreach`" won't apply here -only over ranges / `opApply`s.
 Same answer for the rest of examples with tuples.
No, static and non-static foreach are completely different semantically. My examples may show the same outputs either way, but it's easy to make cases which show the distinction between static and non-static.
See above.
?
I meant that so far, I've seen no diffenece in behaviour of `foreach` and `static foreach` when the aggregate is an `AliasSeq`, except that compilation failure i talked above. I'm not arguing that behaviour should remain the same, just that this appears to be the intended behaviour of them currently. If it is not, sorry, I still don't get it.
 It should effectively be the same as:

 int[3] x;
 static foreach (i; AliasSeq!(x[0], x[1], x[2])) {}

 Which doesn't work, but this does:

 int x, y, z;
 static foreach (i; AliasSeq!(x, y, z)) {}

 This also works:

 int[3] x;
 static foreach (i; 0 .. 3) { ... x[i] ... }

 Unrolling an array is super useful, we just don't support it... 
 but we could!

 [snip]
x is right there, or the code couldn't compile. Our meta is perfectly fine with symbol aliases. For the non-ref (ie, alias) case, sub the array expansion with `(x, y, z)` instead of `(x[0], x[1], x[2])`. It's exactly the same thing, it's just not supported is all. From there, whether we support static foreach fabricating a ref that points to an alias is a different question, but it's totally possible. It's not any different for the compiler than this: void fun(ref int); int x; static foreach(ref i; AliasSeq!(x, x, x)) {} // <- i is a reference to a local fun(x); fun(x); fun(x); // <- function receives a reference to local It makes ref to x in both cases.
 [snip]
It's just that you specifically mention static foreach and that it's meaningless; which I'm not sure is true. You should either not mention static foreach (your DIP applies to runtime foreach), or should say what static foreach should support, and that may be "static foreach does not interact with ref", but it's possible that it could, and could be useful in the future. I think there's just a few gaps still where things that should work haven't been requested. Loop unrolling primarily. We can't alias expressions at all.
Okay, I'll say that `static foreach` with `auto ref` has no effect as long as compile-time lvalues are not possible, but should change the behaviour accordingly if they are possible in the future. And remember, this clause does not apply to alias sequences.
Aug 15
parent reply Manu <turkeyman gmail.com> writes:
On Thu, Aug 15, 2019 at 1:16 AM Dukc via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On Wednesday, 14 August 2019 at 18:26:21 UTC, Manu wrote:
 You need -preview=rvalueRefParam
 I pointed you to Andrei's lecture that talks about this at the
 start
 of the thread.
Now THAT was news to me! I had flipped through the slides of that lecture some day but I didn't remember we had such a review switch. I definitely need to address that somehow. To be frank, I may have to recommend considering alternative #1 instead of the DIP if that switch is planned to be mainstream.
 It does this:

 alias i = tuple[0];
 ... body
 alias i = tuple[1];
 ... body
 alias i = tuple[2];
 ... body

 This is why the ref function arg shows ref correctly. It
 unrolls the loop for each element of the tuple as an alias.
Yeah, but for some reason a plain `foreach` will compile over an `AliasSeq` with ref (if all elements are lvalues), but `static foreach` won't (according to my tests). Might be a bug, the error message seemed strange. I suppose that `foreach` and `static foreach` are intended behave exactly the same in this case. Before considering your proposal of deprecating non-static `foreach` for this use, that is.
I don't think they behave exactly the same in anyAliasSeq case. static foreach over an AliasSeq has an `alias` iterator. foreach ever has an alias iterator.
 I am going to mention that `static foreach` over alias sequence
 behaves exactly as without the keyword. I'll also make clear that
 "no rvalues possible for `static foreach`" won't apply here -only
 over ranges / `opApply`s.
It doesn't behave exactly the same, it's quite different, and I showed that in code a few posts back.
 Same answer for the rest of examples with tuples.
No, static and non-static foreach are completely different semantically. My examples may show the same outputs either way, but it's easy to make cases which show the distinction between static and non-static.
See above.
?
I meant that so far, I've seen no diffenece in behaviour of `foreach` and `static foreach` when the aggregate is an `AliasSeq`, except that compilation failure i talked above.
One is `alias`, one is `auto`. They're semantically VERY different, your meta in the loop body will behave differently.
 I'm not arguing that behaviour should remain the same, just that
 this appears to be the intended behaviour of them currently.

 If it is not, sorry, I still don't get it.
void fun(ref int x) { auto y = x; // remember x foreach (i; AliasSeq!(x, x)) { ++i; } assert(x == y); // only copies were incremented, x is unchanged static foreach (i; AliasSeq!(x, x)) { ++i; } assert(x == y + 2); // the aliased symbol 'x' was incremented }
 It should effectively be the same as:

 int[3] x;
 static foreach (i; AliasSeq!(x[0], x[1], x[2])) {}

 Which doesn't work, but this does:

 int x, y, z;
 static foreach (i; AliasSeq!(x, y, z)) {}

 This also works:

 int[3] x;
 static foreach (i; 0 .. 3) { ... x[i] ... }

 Unrolling an array is super useful, we just don't support it...
 but we could!

 [snip]
x is right there, or the code couldn't compile. Our meta is perfectly fine with symbol aliases. For the non-ref (ie, alias) case, sub the array expansion with `(x, y, z)` instead of `(x[0], x[1], x[2])`. It's exactly the same thing, it's just not supported is all. From there, whether we support static foreach fabricating a ref that points to an alias is a different question, but it's totally possible. It's not any different for the compiler than this: void fun(ref int); int x; static foreach(ref i; AliasSeq!(x, x, x)) {} // <- i is a reference to a local fun(x); fun(x); fun(x); // <- function receives a reference to local It makes ref to x in both cases.
 [snip]
It's just that you specifically mention static foreach and that it's meaningless; which I'm not sure is true. You should either not mention static foreach (your DIP applies to runtime foreach), or should say what static foreach should support, and that may be "static foreach does not interact with ref", but it's possible that it could, and could be useful in the future. I think there's just a few gaps still where things that should work haven't been requested. Loop unrolling primarily. We can't alias expressions at all.
Okay, I'll say that `static foreach` with `auto ref` has no effect as long as compile-time lvalues are not possible, but should change the behaviour accordingly if they are possible in the future.
Compile-time lvalues are absolutely possible, I've showed code how it works a bunch of times, including just above (difference between foreach and static foreach).
 And remember, this clause does not apply to alias
 sequences.
It's a bit weird to say 'except' that way; static foreach primarily applies to tuples, and foreach should probably be deprecated such that it can't apply to tuples. It's not technically wrong, but it's just seems a bit of a weird way to paint that rule to me.
Aug 15
parent reply Dukc <ajieskola gmail.com> writes:
On Friday, 16 August 2019 at 05:31:38 UTC, Manu wrote:
 On Thu, Aug 15, 2019 at 1:16 AM Dukc via Digitalmars-d
 If it is not, sorry, I still don't get it.
void fun(ref int x) { auto y = x; // remember x foreach (i; AliasSeq!(x, x)) { ++i; } assert(x == y); // only copies were incremented, x is unchanged static foreach (i; AliasSeq!(x, x)) { ++i; } assert(x == y + 2); // the aliased symbol 'x' was incremented }
Yes, this works. Disregard all what I have said about `static foreach` over an `AliasSeq`, I obviously need to think it completely again.
 Okay, I'll say that `static foreach` with `auto ref` has no 
 effect as long as compile-time lvalues are not possible, but 
 should change the behaviour accordingly if they are possible 
 in the future.
Compile-time lvalues are absolutely possible, I've showed code how it works a bunch of times, including just above (difference between foreach and static foreach).
Yeah, as an `AliasSeq`, but I meant ranges and structs with an `opApply()` here. I know I didn't say it that way in the DIP, but I already agreed to change it.
 And remember, this clause does not apply to alias sequences.
It's a bit weird to say 'except' that way; static foreach primarily applies to tuples, and foreach should probably be deprecated such that it can't apply to tuples. It's not technically wrong, but it's just seems a bit of a weird way to paint that rule to me.
I'll try to avoid describing it this way in the DIP, it's just that it was easiest to answer quote-by-quote in this theard and that led to my articulation of "except".
Aug 17
parent reply Manu <turkeyman gmail.com> writes:
On Sat, Aug 17, 2019 at 12:58 AM Dukc via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On Friday, 16 August 2019 at 05:31:38 UTC, Manu wrote:
 On Thu, Aug 15, 2019 at 1:16 AM Dukc via Digitalmars-d
 If it is not, sorry, I still don't get it.
void fun(ref int x) { auto y = x; // remember x foreach (i; AliasSeq!(x, x)) { ++i; } assert(x == y); // only copies were incremented, x is unchanged static foreach (i; AliasSeq!(x, x)) { ++i; } assert(x == y + 2); // the aliased symbol 'x' was incremented }
Yes, this works. Disregard all what I have said about `static foreach` over an `AliasSeq`, I obviously need to think it completely again.
 Okay, I'll say that `static foreach` with `auto ref` has no
 effect as long as compile-time lvalues are not possible, but
 should change the behaviour accordingly if they are possible
 in the future.
Compile-time lvalues are absolutely possible, I've showed code how it works a bunch of times, including just above (difference between foreach and static foreach).
Yeah, as an `AliasSeq`, but I meant ranges and structs with an `opApply()` here. I know I didn't say it that way in the DIP, but I already agreed to change it.
 And remember, this clause does not apply to alias sequences.
It's a bit weird to say 'except' that way; static foreach primarily applies to tuples, and foreach should probably be deprecated such that it can't apply to tuples. It's not technically wrong, but it's just seems a bit of a weird way to paint that rule to me.
I'll try to avoid describing it this way in the DIP, it's just that it was easiest to answer quote-by-quote in this theard and that led to my articulation of "except".
👍 I think we're on the same page now. I'll stop hassling and let you make those tweaks :)
Aug 17
parent Dukc <ajieskola gmail.com> writes:
On Saturday, 17 August 2019 at 08:08:44 UTC, Manu wrote:
 👍

 I think we're on the same page now.
 I'll stop hassling and let you make those tweaks :)
Thanks again. You revealed a lot of stuff.
Aug 18
prev sibling parent Nick Treleaven <nick geany.org> writes:
On Tuesday, 13 August 2019 at 23:34:32 UTC, Manu wrote:
 On Tue, Aug 13, 2019 at 6:50 AM Dukc via Digitalmars-d 
 <digitalmars-d puremagic.com> wrote:
   static foreach (ref i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 true true true
Yes, these are the cases I meant with `static foreach`. I think `[10, 20, 30]` is a rvalue, and if the latter of these two examples compiles, it should not.
Actually, because of D's 'weird shit' law, the array is not an rvalue. (I think...?)
An array literal is an rvalue: void f(ref int[] a); void main(){ f([10, 20, 30]); // error } error: cannot pass rvalue argument `[10, 20, 30]` of type `int[]` to parameter `ref int[] a`
 As above, we could allow this by creating temporaries the 
 same as function arguments... but we've decided not to allow 
 ref iterators from rvalues.

   static foreach (auto ref i; [10, 20, 30])
       pragma(msg, __traits(isRef, i));

 false false false ??
Yes, exactly what's supposed to happen.
Right... what's your point? Incidentally, I'm not sure this will hold; because array literals are not rvalues, so I think it might be `true true true` here...
An array literal is an rvalue, but indexing an array literal is an lvalue. So the above should print 'true' for each pragma.
Aug 14
prev sibling next sibling parent reply rikki cattermole <rikki cattermole.co.nz> writes:
- 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
Aug 08
parent reply Dukc <ajieskola gmail.com> writes:
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?
Aug 08
next sibling parent reply rikki cattermole <rikki cattermole.co.nz> writes:
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.
Aug 08
parent Dukc <ajieskola gmail.com> writes:
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.
Aug 08
prev sibling parent Dukc <ajieskola gmail.com> writes:
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?
Aug 08
prev sibling parent Dukc <ajieskola gmail.com> writes:
I just got an email that this review is over. The changes I'm 
going to make are large enough that my plan is request another 
community review round after I've done my revisions.

Thanks you Mike, and thanks for all reviewers.
Aug 23