Re: Unexpected foreach lowering
On Thursday, August 11, 2016 08:42:27 Steven Schveighoffer via Digitalmars-d- learn wrote: > On 8/11/16 12:28 AM, Jonathan M Davis via Digitalmars-d-learn wrote: > > On Wednesday, August 10, 2016 21:00:01 Lodovico Giaretta via > > Digitalmars-d- > > > > learn wrote: > >> Wow. Thanks. I didn't know the compiler would try opSlice. I will > >> file it. > > > > It does that so that you can use foreach with containers without having to > > call something on the container. The idea is that the container will > > implement opSlice and make it return a range over the container, and > > foreach will then use that range to iterate over the container. > > I get that. But it shouldn't try opSlice *first* if the item itself is a > range (and it does do this). Many random-access ranges define opSlice, > and most of the time range[] returns this. But in this case, it doesn't. > > But it's a no-op for ranges, why waste time calling it? I was just explaining what the deal with opSlice and containers was, since the OP wasn't familiar with it. I wasn't really saying anything about the suggested change. However, I have to agree that the suggested change is likely a good one. As it stands, no range should be implementing opSlice with no arguments, since it's a container function, not a range function. No range trait tests for it, and no range-based code should be using it. It's just supposed to be used on containers to get a range not on the ranges themselves. But some folks put it on ranges anyway (IIRC, there are even at least a couple of cases in Phobos where opSlice is implemented on ranges when it shouldn't be), and it causes problems. There might be a reason why it would be a bad idea to change it so that opSlice is ignored by foreach if the type is a range, but I can't think of any right now. Unfortunately, ranges and foreach have a tendency to get a bit funny thanks to the differing behavior between types (e.g. ranges that are implicitly saved when used with foreach vs those that aren't), so mucking around with it should be done with care, but it at least seems like your suggestion to skip opSlice if the range primitives are there is a good one. - Jonathan M Davis
Re: Unexpected foreach lowering
On 8/10/16 5:14 PM, ag0aep6g wrote: On 08/10/2016 10:54 PM, Steven Schveighoffer wrote: The issue is that it tries using [] on the item to see if it defines a range-like thing. Since you don't define opSlice(), it automatically goes to the subrange. This breaks for int[] as well as Array. If I add opSlice to your code (and return this) it works. This is definitely a bug, it should try range functions before opSlice. Please file. Related: https://issues.dlang.org/show_bug.cgi?id=14619 That's the same issue. Thanks. Your case is even worse, because calling save could be unnecessary and costly. Note that the compiler will turn this: foreach(...; foo[]) into this: foreach(...; foo[][]) ugly... -Steve
Re: Unexpected foreach lowering
On 8/11/16 12:28 AM, Jonathan M Davis via Digitalmars-d-learn wrote: On Wednesday, August 10, 2016 21:00:01 Lodovico Giaretta via Digitalmars-d- learn wrote: Wow. Thanks. I didn't know the compiler would try opSlice. I will file it. It does that so that you can use foreach with containers without having to call something on the container. The idea is that the container will implement opSlice and make it return a range over the container, and foreach will then use that range to iterate over the container. I get that. But it shouldn't try opSlice *first* if the item itself is a range (and it does do this). Many random-access ranges define opSlice, and most of the time range[] returns this. But in this case, it doesn't. But it's a no-op for ranges, why waste time calling it? -Steve
Re: Unexpected foreach lowering
On Wednesday, August 10, 2016 21:00:01 Lodovico Giaretta via Digitalmars-d- learn wrote: > Wow. Thanks. I didn't know the compiler would try opSlice. I will > file it. It does that so that you can use foreach with containers without having to call something on the container. The idea is that the container will implement opSlice and make it return a range over the container, and foreach will then use that range to iterate over the container. - Jonathan M Davis
Re: Unexpected foreach lowering
On 08/10/2016 10:54 PM, Steven Schveighoffer wrote: The issue is that it tries using [] on the item to see if it defines a range-like thing. Since you don't define opSlice(), it automatically goes to the subrange. This breaks for int[] as well as Array. If I add opSlice to your code (and return this) it works. This is definitely a bug, it should try range functions before opSlice. Please file. Related: https://issues.dlang.org/show_bug.cgi?id=14619
Re: Unexpected foreach lowering
On Wednesday, 10 August 2016 at 21:00:01 UTC, Lodovico Giaretta wrote: On Wednesday, 10 August 2016 at 20:54:15 UTC, Steven Schveighoffer wrote: [...] Wow. Thanks. I didn't know the compiler would try opSlice. I will file it. Filed on bugzilla: https://issues.dlang.org/show_bug.cgi?id=16374
Re: Unexpected foreach lowering
On Wednesday, 10 August 2016 at 20:54:15 UTC, Steven Schveighoffer wrote: On 8/10/16 2:08 PM, Lodovico Giaretta wrote: [...] The issue is that it tries using [] on the item to see if it defines a range-like thing. Since you don't define opSlice(), it automatically goes to the subrange. This breaks for int[] as well as Array. If I add opSlice to your code (and return this) it works. This is definitely a bug, it should try range functions before opSlice. Please file. -Steve Wow. Thanks. I didn't know the compiler would try opSlice. I will file it.
Re: Unexpected foreach lowering
On 8/10/16 2:08 PM, Lodovico Giaretta wrote:
I'm probably missing something stupid but...
Why on earth do the two loops in main print a different result?
It looks like the foreach lowering is ignoring my definition of front...
=
import std.stdio, std.container.array;
struct RangeWrapper(Range)
{
Range range;
alias range this;
auto front()
{
return range.front + 1;
}
}
auto rangeWrapper(Range)(auto ref Range range)
{
return RangeWrapper!Range(range);
}
void main()
{
Array!int array;
array.insertBack(3);
foreach (i; rangeWrapper(array[]))
writeln(i); // prints 3, which is wrong
// isn't the above foreach equivalent to the following loop ?
for (auto r = rangeWrapper(array[]); !r.empty; r.popFront())
writeln(r.front); // correctly prints 4
}
=
Thank you for your help.
The issue is that it tries using [] on the item to see if it defines a
range-like thing. Since you don't define opSlice(), it automatically
goes to the subrange.
This breaks for int[] as well as Array.
If I add opSlice to your code (and return this) it works.
This is definitely a bug, it should try range functions before opSlice.
Please file.
-Steve
Re: Unexpected foreach lowering
On Wednesday, 10 August 2016 at 19:37:39 UTC, Ali Çehreli wrote: A quick read reveals popFront() is implemented only for bool Arrays. That explains the issue. I don't know whether it's an oversight. Ali First of all, thank you for spending your time on this issue. I really appreciate that. I may be reading that code wrong but... Isn't popFront implemented for every type here? https://github.com/dlang/phobos/blob/master/std/container/array.d#L128
Re: Unexpected foreach lowering
On 08/10/2016 11:47 AM, Lodovico Giaretta wrote:
On Wednesday, 10 August 2016 at 18:38:00 UTC, Ali Çehreli wrote:
RangeWrapper does not provide the InputRange interface, so the
compiler uses 'alias this' and iterates directly on the member range.
I tried making RangeWrapper an InputRange but failed. It still uses
'range'.
// Still fails with these:
@property bool empty() {
return range.empty;
}
void popFront() {
range.popFront();
}
I don't know how the decision process works there.
Ali
That's strange, as RangeWrapper works correctly if instantiated with any
underlying range EXCEPT std.container.Array.
A quick read reveals popFront() is implemented only for bool Arrays.
That explains the issue.
I don't know whether it's an oversight.
Ali
Re: Unexpected foreach lowering
On Wednesday, 10 August 2016 at 18:38:00 UTC, Ali Çehreli wrote:
RangeWrapper does not provide the InputRange interface, so the
compiler uses 'alias this' and iterates directly on the member
range.
I tried making RangeWrapper an InputRange but failed. It still
uses 'range'.
// Still fails with these:
@property bool empty() {
return range.empty;
}
void popFront() {
range.popFront();
}
I don't know how the decision process works there.
Ali
That's strange, as RangeWrapper works correctly if instantiated
with any underlying range EXCEPT std.container.Array.
Also, RangeWrapper does provide the InputRange interface,
partially directly and partially with alias this. RangeWrapper
should be "opaque", as it should not matter whether the methods
needed for the InputRange interface are defined directly or
inherited with alias this.
Re: Unexpected foreach lowering
On 08/10/2016 11:08 AM, Lodovico Giaretta wrote:
> I'm probably missing something stupid but...
> Why on earth do the two loops in main print a different result?
> It looks like the foreach lowering is ignoring my definition of front...
>
> =
> import std.stdio, std.container.array;
>
> struct RangeWrapper(Range)
> {
> Range range;
> alias range this;
>
> auto front()
> {
> return range.front + 1;
> }
> }
> auto rangeWrapper(Range)(auto ref Range range)
> {
> return RangeWrapper!Range(range);
> }
>
> void main()
> {
> Array!int array;
> array.insertBack(3);
>
> foreach (i; rangeWrapper(array[]))
> writeln(i); // prints 3, which is wrong
>
> // isn't the above foreach equivalent to the following loop ?
>
> for (auto r = rangeWrapper(array[]); !r.empty; r.popFront())
> writeln(r.front); // correctly prints 4
> }
> =
>
> Thank you for your help.
RangeWrapper does not provide the InputRange interface, so the compiler
uses 'alias this' and iterates directly on the member range.
I tried making RangeWrapper an InputRange but failed. It still uses 'range'.
// Still fails with these:
@property bool empty() {
return range.empty;
}
void popFront() {
range.popFront();
}
I don't know how the decision process works there.
Ali
Re: Unexpected foreach lowering
On Wednesday, 10 August 2016 at 18:08:02 UTC, Lodovico Giaretta
wrote:
I'm probably missing something stupid but...
Why on earth do the two loops in main print a different result?
It looks like the foreach lowering is ignoring my definition of
front...
=
import std.stdio, std.container.array;
struct RangeWrapper(Range)
{
Range range;
alias range this;
auto front()
{
return range.front + 1;
}
}
auto rangeWrapper(Range)(auto ref Range range)
{
return RangeWrapper!Range(range);
}
void main()
{
Array!int array;
array.insertBack(3);
foreach (i; rangeWrapper(array[]))
writeln(i); // prints 3, which is wrong
// isn't the above foreach equivalent to the following
loop ?
for (auto r = rangeWrapper(array[]); !r.empty; r.popFront())
writeln(r.front); // correctly prints 4
}
=
Thank you for your help.
This actually only happens with std.container.Array. Other ranges
are ok. Even stranger...
