Bug 6652 – foreach parameter with number range is always ref
Status
RESOLVED
Resolution
FIXED
Severity
enhancement
Priority
P2
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2011-09-12T10:19:00Z
Last change time
2013-11-01T12:16:23Z
Keywords
pull, wrong-code
Assigned to
nobody
Creator
code
Comments
Comment #0 by code — 2011-09-12T10:19:15Z
void main() {
size_t cnt;
foreach(ulong n; 0 .. 10)
{
++n;
++cnt;
}
assert(cnt == 10);
cnt = 0;
foreach(ref ulong n; 0 .. 10)
{
++n;
++cnt;
}
assert(cnt == 5);
}
---
As this is rewritten in terms of a for loop all writes to n will
alter the loop.
A hidden copy of n is needed for non-ref parameters to match the range
foreach semantic.
Comment #1 by bearophile_hugs — 2011-09-12T12:24:41Z
Recently I have started a very long thread about this problem:
http://www.digitalmars.com/d/archives/digitalmars/D/About_foreach_loops_138630.htmlhttp://www.digitalmars.com/d/archives/digitalmars/D/Re_About_foreach_loops_138666.html
An alternative and probably a bit better solution is to think of 0..10 as a immutable entity (just like the integer number "1" is immutable, likewise a range of numbers is immutable), so the foreach index is a const (the compiler keeps only one index, for efficiency, and modifies this const index):
foreach (int i; 0 .. 10) {
i++; // forbidden, i is const
}
If you want to modify the index variable inside the loop, then you use a for() loop.
Comment #2 by code — 2011-09-12T14:35:51Z
Making a const/immutable copy is not the right solution to this.
Instead a mutable copy of a hidden loop variable should be made.
Being a copy is the common behavior for non-ref foreach arguments,
to my surprise it has even become my intuitive assumption of what's happening.
The old behavior can be achieved through a ref argument.
not possible using const:
foreach(i; 1 .. 10)
while(i--) { do some }
Comment #3 by bearophile_hugs — 2011-09-12T15:10:08Z
(In reply to comment #2)
> Making a const/immutable copy is not the right solution to this.
Keep in mind that foreach(i;0..10) must have *zero* abstraction penalty over a for loop even with non-optimizing D compilers, because it's meant to replace for loops everywhere possible. The more abstraction you put into foreach there higher the probability it will not have zero abstraction penalty (currently it has a bit of penalty for nested loops, sometimes).
> not possible using const:
> foreach(i; 1 .. 10)
> while(i--) { do some }
Use a for loop :-)
Comment #4 by code — 2011-09-13T00:02:29Z
https://github.com/D-Programming-Language/dmd/pull/377
Foreach arguments behave like function arguments. Here they don't.
The variable can be optimized out, if no altering happens.
This will not happen in a debug build, where it is irrelevant in comparison to every variable being accessed through the stack.
You can use ref, if you're having too expensive copies
foreach(ref const i; iter(0) .. iter(10)) as with every other foreach argument.
Most important it has an explicit rule, that one can alter the loop index through using a ref index.
foreach(ref idx, v; [0, 1, 2, 3, 4, 5])
idx += stride - 1;
Comment #5 by bearophile_hugs — 2011-09-13T03:59:49Z
(In reply to comment #4)
> https://github.com/D-Programming-Language/dmd/pull/377
> - I've double checked that a simple size_t index is optimized out if unaltered
I suggest you to check it fifteen more times, using 4 nested foreach, with some code inside the bodies, with other data types (ubyte, short, ulong, real, double, etc), etc.
Comment #6 by code — 2011-09-13T21:49:08Z
It really is not that much an issue of performance.
The compiler should be able to eliminate dead assignments
and *& for ref parameters.
The issue is that of breaking code. I don't know any feasible
solution to attach a deprecation to this now and change it later.
Comment #7 by repeatedly — 2012-06-11T08:47:41Z
I hit this issue today.
Current behavior is different from foreach semantics.
So, I agree dawg opinion.
We should fix this bug!
Comment #9 by bearophile_hugs — 2012-06-15T11:56:35Z
(In reply to comment #8)
> To reduce breaking of existing codes,
> 1. Warn to modifying loop variable in foreach body.
> It is shown only when -w switch is specified.
> 2. Deprecate modifying loop variable in foreach body.
> If user specifies -d switch, it is allowed.
> 3. Allow modifying loop variable in foreach body, and it does not affect to
> the number of iterations of the loop.
This is great, you are the best Kenji Hara.
I prefer the number 2. I think it breaks none of my programs.
The number 3 is a trap, because it silently changes the semantics of old D code. And it's bug-prone for new D programmers too because they can change the variable by mistake. Generally immutable variables are safer.
Are you able and willing to compile the whole Phobos with the option number 2? So we can see how often Phobos code change the foreach-on-range iteration variable.
What's the semantics of this, now?
foreach (ref int i; 0 .. 10) i++;
Comment #10 by k.hara.pg — 2012-06-15T19:40:31Z
(In reply to comment #9)
> (In reply to comment #8)
>
> > To reduce breaking of existing codes,
> > 1. Warn to modifying loop variable in foreach body.
> > It is shown only when -w switch is specified.
> > 2. Deprecate modifying loop variable in foreach body.
> > If user specifies -d switch, it is allowed.
> > 3. Allow modifying loop variable in foreach body, and it does not affect to
> > the number of iterations of the loop.
>
> This is great, you are the best Kenji Hara.
>
> I prefer the number 2. I think it breaks none of my programs.
They are the phases to change behavior. I think we should allow modifying loop variable in foreach body, but it should not affect to iteration.
> The number 3 is a trap, because it silently changes the semantics of old D
> code. And it's bug-prone for new D programmers too because they can change the
> variable by mistake. Generally immutable variables are safer.
#3 is a goal.
> Are you able and willing to compile the whole Phobos with the option number 2?
> So we can see how often Phobos code change the foreach-on-range iteration
> variable.
http://d.puremagic.com/test-results/pulls.ghtml
See auto tester. With all pull request, Phobos compile succeeds. So there is no code that changes the foreach-on-range iteration variable.
Comment #11 by r.97all — 2012-06-15T19:57:43Z
(In reply to comment #9)
> I prefer the number 2. I think it breaks none of my programs.
>
> The number 3 is a trap, because it silently changes the semantics of old D
> code. And it's bug-prone for new D programmers too because they can change the
> variable by mistake. Generally immutable variables are safer.
In my point of view, as a newcomer to D, more bug-prone is the current behavior.
Foreach statement provides iteration over arrays, ranges, etc, and notation of range "0..n" also *looks like* a collection. So, foreach range statements should work like foreach over collection.
I have wrote to stuck in my program:
foreach (i; 0..M^^n)
{
foreach (j; 0..n)
{
a[j] = i % M;
i /= M;
}
// operations which use a but i
}
which I wrote in Python before:
for i in range(M**n):
for j in range(n):
a[j] = i % M
i /= M
# operations which use a but i
and was sad to see an infinite loop.
Even a new programmer *intends* to change the value of i when changing, if not just a typo.
If someone want to affect loop, s/he can write
i = 0;
while (i < 10)
{
// operations which change the value of i
i += 1;
}
Comment #12 by bearophile_hugs — 2012-06-16T05:07:21Z
(In reply to comment #10)
> They are the phases to change behavior.
I see.
> I think we should allow modifying loop
> variable in foreach body, but it should not affect to iteration.
Generally changing the iteration variable isnt't a very good idea. It looks bug-prone, like modifying function arguments inside functions :-)
foreach (i; 0 .. 10) { i++; writeln(i); }
> So there is no
> code that changes the foreach-on-range iteration variable.
Good.
Comment #13 by bearophile_hugs — 2012-06-16T05:14:39Z
If phase 3 will be accepted, I hope this syntax too will be accepted:
foreach (const i; 0 .. 10) {}
Comment #14 by bearophile_hugs — 2012-06-16T05:15:07Z
(In reply to comment #11)
> In my point of view, as a newcomer to D, more bug-prone is the current
> behavior.
Of course. But here the comparison wasn't between the current behavour and the phase 3. It was mostly a comparison between the phase 2 and phase 3.
Comment #15 by k.hara.pg — 2012-06-16T05:27:32Z
(In reply to comment #13)
> If phase 3 will be accepted, I hope this syntax too will be accepted:
>
> foreach (const i; 0 .. 10) {}
I think it should be allowed.
Comment #16 by r.97all — 2012-06-16T09:00:32Z
(In reply to comment #14)
> > In my point of view, as a newcomer to D, more bug-prone is the current
> > behavior.
> Of course. But here the comparison wasn't between the current behavour and the
> phase 3. It was mostly a comparison between the phase 2 and phase 3.
What followed it was my opinion to it:
> > Even a new programmer *intends* to change the value of i when changing, if not just a typo.
The problem is that changing the value of a variable affects loop, not changing itself.
Comment #17 by bearophile_hugs — 2012-06-17T15:31:45Z
Just a note.
void main() {
import std.stdio;
auto array = [10, 20, 30, 40, 50];
foreach (i, item; array) {
writeln(item);
i++;
}
}
Currently it prints:
10
30
50
Comment #18 by code — 2012-06-19T06:10:15Z
>How about?
Sounds great. It doesn't break code and allows us to fix this finally.
>foreach (i, item; array)
Yeah, it should apply to the index variable as well.
Comment #19 by k.hara.pg — 2012-06-21T08:32:51Z
(In reply to comment #18)
> >How about?
> Sounds great. It doesn't break code and allows us to fix this finally.
>
> >foreach (i, item; array)
> Yeah, it should apply to the index variable as well.
Comment #20 by k.hara.pg — 2012-06-21T08:35:11Z
(In reply to comment #19)
> (In reply to comment #18)
> > >How about?
> > Sounds great. It doesn't break code and allows us to fix this finally.
> >
> > >foreach (i, item; array)
> > Yeah, it should apply to the index variable as well.
(Sorry, I accidentally pressed the "commit" button...)
OK. Now, my pull requests also care the key of array in iteration.
You can see the test case:
https://github.com/D-Programming-Language/dmd/pull/1008/files#L5L-1
Comment #21 by github-bugzilla — 2012-06-21T14:21:51Z