www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - opApply not called for foeach(container)

reply "monarch_dodra" <monarch_dodra gmail.com> writes:
If you create a class/struct that can give you a (forward) range 
via "opSlice()", and that range gives you access to "opApply", 
then you get two different behaviors:

----
MyClass arr = ...;

foreach(a; arr)
    ...

foreach(a; arr[])
    ...

----
In the first case, foreach will call opSlice(), and then walk 
through the resulting arr[] range with the front/popFront/empty 
troika.

In the second case, foreach will call opApply on the range 
"arr[]".

----
I'm wondering if this is the correct behavior? In particular, 
since foreach guarantees a call to opSlice(), so writing "arr[]" 
*should* be redundant, yet the final behavior is different.

That said, the "issue" *could* be fixed if the base class defines 
opApply as: "return opSlice().opApply(dg)" (or more complex). 
However:
a) The implementer of class has no obligation to do this, since 
he has provided a perfectly valid range.
b) This would force implementers into more generic useless 
boilerplate code.

What are your thoughts? Which is the "correct" solution? Is it a 
bug with foreach, or should the base struct/class provide an 
opApply?

PS: Related: "foreach over range (with opApply) should save 
range."
http://d.puremagic.com/issues/show_bug.cgi?id=4347
On this assigned issue, is the conclusion that foreach will 
eventually save the range?
Jul 11 2012
next sibling parent reply travert phare.normalesup.org (Christophe Travert) writes:
"monarch_dodra" , dans le message (digitalmars.D:171868), a crit:

 I'm wondering if this is the correct behavior? In particular, 
 since foreach guarantees a call to opSlice(), so writing "arr[]" 
 *should* be redundant, yet the final behavior is different.
 
 That said, the "issue" *could* be fixed if the base class defines 
 opApply as: "return opSlice().opApply(dg)" (or more complex). 
 However:
 a) The implementer of class has no obligation to do this, since 
 he has provided a perfectly valid range.
 b) This would force implementers into more generic useless 
 boilerplate code.
 
 What are your thoughts? Which is the "correct" solution? Is it a 
 bug with foreach, or should the base struct/class provide an 
 opApply?


I think foreach should never call opSlice. That's not in the online 
documentation (http://dlang.org/statement.html#ForeachStatement), unless 
I missed something. If you want to use foreach on a class with an 
opSlice, then yes, you should define opApply. Otherwise, the user have 
to call opSlice himself, which seems reasonable. That's how I understand 
the doc.

-- 
Christophe
Jul 11 2012
next sibling parent reply "monarch_dodra" <monarch_dodra gmail.com> writes:
On Wednesday, 11 July 2012 at 14:10:35 UTC, 
travert phare.normalesup.org (Christophe Travert) wrote:

 I think foreach should never call opSlice. That's not in the 
 online
 documentation 
 (http://dlang.org/statement.html#ForeachStatement), unless
 I missed something. If you want to use foreach on a class with 
 an
 opSlice, then yes, you should define opApply. Otherwise, the 
 user have
 to call opSlice himself, which seems reasonable. That's how I 
 understand
 the doc.


Hum... One thing is for sure, it _does_ call opSlice when it is 
defined.

I just re-read the docs you linked to, and if that was my only 
source, I'd reach the same conclusion as you. however, my "The D 
Programming Language", states:
*12: Operator Overloading
**9: Overloading foreach
***1: foreach with Iteration Primitives
"Last but not least, if the iterated object offers the slice 
operator with no arguments lst[], __c is initialized with lst[] 
instead of lst. This is in order to allow “extracting” the 
iteration means out of a container without requiring the 
container to define the three iteration primitives."

Another thing I find strange about the doc is: "If the foreach 
range properties do not exist, the opApply method will be used 
instead." This sounds backwards to me.
Jul 11 2012
parent reply travert phare.normalesup.org (Christophe Travert) writes:
"monarch_dodra" , dans le message (digitalmars.D:171902), a crit:

 I just re-read the docs you linked to, and if that was my only 
 source, I'd reach the same conclusion as you.


I think the reference spec for D should be the community driven and 
widely available website, not a commercial book. But that's not the 
issue here.


 however, my "The D 
 Programming Language", states:
 *12: Operator Overloading
 **9: Overloading foreach
 ***1: foreach with Iteration Primitives
 "Last but not least, if the iterated object offers the slice 
 operator with no arguments lst[], __c is initialized with lst[] 
 instead of lst. This is in order to allow ?extracting? the 
 iteration means out of a container without requiring the 
 container to define the three iteration primitives."
 
 Another thing I find strange about the doc is: "If the foreach 
 range properties do not exist, the opApply method will be used 
 instead." This sounds backwards to me.


Skipping the last paragraph, a reasonable interpretation would be that 
foreach try to use, in order of preference:
 - for each over array
 - opApply
 - the three range primitives (preferably four if we include save)
 - opSlice (iteration on the result of opSlice is determined by the same 
system).

 opApply should come first, since if someone defines opApply, he or she 
obviously wants to override the range primitive iteration.
 opApply and range primitives may be reached via an alias this.
 opSlice is called only if no way to iterate the struct/class is found. 
 I would not complain if the fourth rule didn't exist, because it's not 
described in dlang.org, but it is a reasonable feature that have be 
taken from TDPL (but then it should be added in dlang.org).


This way, if arr is a container that defines an opSlice, and that does 
not define an opApply, and does not define range primitives:

foreach (a, arr) ...
and
foreach (a, arr[]) ...
should be stricly equivalent. Since the first is translated into the 
second. Both work only if arr[] is iterable.

I think you hit a bug.

-- 
Christophe
Jul 11 2012
parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, July 11, 2012 15:43:13 Christophe Travert wrote:

 "monarch_dodra" , dans le message (digitalmars.D:171902), a écrit :

 I just re-read the docs you linked to, and if that was my only
 source, I'd reach the same conclusion as you.


 
 I think the reference spec for D should be the community driven and
 widely available website, not a commercial book. But that's not the
 issue here.


The problem is that it's essentially spread out across 3 places: the online 
spec, TDPL, and the compiler. The online spec is _supposed_ to have all of the 
information and be correct, but sometimes behavior and/or planned behavior has 
been changed without properly updating the online spec, and sometimes the 
online spec just doesn't include enough information. TDPL includes intended 
behavior, which in most cases is correct and matches what the compiler 
currently does, but in some cases it includes features which just haven't been 
fully implemented yet (e.g. having multiple alias thises). The compiler is of 
course the current behavior.

So, when they don't match, we have to figure out what the intended behavior is 
and, in some cases, what we now want the behavior to be (regardless of what we 
intended before).

The source for the website is freely available ( https://github.com/D-
Programming-Language/d-programming-language.org ) - including the spec - and 
anyone can create a pull request for it. The main problem is probably a 
combination of the fact that the people who know enough to fix it are very busy 
with other things (like fixing compiler bugs) and the fact that they aren't 
always aware of what needs fixing. They're also much more likely to interpret 
what's there as was intended rather than what a newbie would take it to mean 
and not realize that it needs improvement. And I suspect that most of the 
people who know what they're doing around here don't actually look at the spec 
much. Raising specific issues when you find them and putting them in bugzilla 
will help.

So, yes, the online spec should have everything needed to specify D in it, but 
making it so requires time and manpower, and for the most part, that has been 
spent on other stuff (much of which is also critical - and often more 
critical).

- Jonathan M Davis
Jul 11 2012
parent reply "monarch_dodra" <monarch_dodra gmail.com> writes:
On Wednesday, 11 July 2012 at 20:24:42 UTC, Jonathan M Davis 
wrote:

 The problem is that it's essentially spread out across 3 
 places: the online
 spec, TDPL, and the compiler.

 ...

 - Jonathan M Davis


The problem is not only the documentation, it's getting 
authoritative answers. I see some strange behaviors, and I can't 
for the life of find out if it is a bug in the 
documentation/compiler, or a pitfall/misunderstanding.

What is your take on the issue at hand? Bug or Pitfall?
Jul 12 2012
next sibling parent Timon Gehr <timon.gehr gmx.ch> writes:
On 07/12/2012 04:50 PM, monarch_dodra wrote:

 On Wednesday, 11 July 2012 at 20:24:42 UTC, Jonathan M Davis wrote:

 The problem is that it's essentially spread out across 3 places: the
 online
 spec, TDPL, and the compiler.

 ...

 - Jonathan M Davis


 The problem is not only the documentation, it's getting authoritative
 answers. I see some strange behaviors, and I can't for the life of find
 out if it is a bug in the documentation/compiler, or a
 pitfall/misunderstanding.

 What is your take on the issue at hand? Bug or Pitfall?


A pitfall is to be considered a bug. Code should have the obvious
behaviour or be illegal.
Jul 12 2012
prev sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, July 12, 2012 16:50:22 monarch_dodra wrote:

 On Wednesday, 11 July 2012 at 20:24:42 UTC, Jonathan M Davis
 
 wrote:

 The problem is that it's essentially spread out across 3
 places: the online
 spec, TDPL, and the compiler.
 
 ...
 
 - Jonathan M Davis


 
 The problem is not only the documentation, it's getting
 authoritative answers. I see some strange behaviors, and I can't
 for the life of find out if it is a bug in the
 documentation/compiler, or a pitfall/misunderstanding.
 
 What is your take on the issue at hand? Bug or Pitfall?


My take on it is that you should either go with ranges or opApply and try to 
avoid mixing them. Ideally, we wouldn't even need opApply anymore, but ranges 
can't quite do everything that it can.

- Jonathan m Davis
Jul 15 2012
parent reply "Mehrdad" <wfunction hotmail.com> writes:
On Sunday, 15 July 2012 at 20:01:35 UTC, Jonathan M Davis wrote:

 Ideally, we wouldn't even need opApply anymore


AFAIT, that's impossible.

Not because of D, but because of other stuff.

Lots of functions require callbacks for enumeration (e.g. 
EnumChildWindows in Windows); it's simply impossible to wrap them 
through something that doesn't take control away from the caller.
Jul 15 2012
parent reply "David Nadlinger" <see klickverbot.at> writes:
On Monday, 16 July 2012 at 06:27:13 UTC, Mehrdad wrote:

 On Sunday, 15 July 2012 at 20:01:35 UTC, Jonathan M Davis wrote:

 Ideally, we wouldn't even need opApply anymore


 AFAIT, that's impossible.

 Not because of D […]


How else woud you implement things like parallel foreach, …?

David
Jul 16 2012
parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Monday, July 16, 2012 09:47:50 David Nadlinger wrote:

 On Monday, 16 July 2012 at 06:27:13 UTC, Mehrdad wrote:

 On Sunday, 15 July 2012 at 20:01:35 UTC, Jonathan M Davis wrote:

 Ideally, we wouldn't even need opApply anymore


=20
 AFAIT, that's impossible.
=20
 Not because of D [=E2=80=A6]


=20
 How else woud you implement things like parallel foreach, =E2=80=A6?


As I said, _ideally_ we wouldn't need opApply, but ranges fail to be ab=
le to=20
work in every use case that opApply does. As great as ranges are, there=
 _are_=20
some cases where ranges are insufficient. So, we still have opApply.

- Jonathan M Davis
Jul 16 2012
parent "David Nadlinger" <see klickverbot.at> writes:
On Monday, 16 July 2012 at 08:13:49 UTC, Jonathan M Davis wrote:

 On Monday, July 16, 2012 09:47:50 David Nadlinger wrote:

 On Monday, 16 July 2012 at 06:27:13 UTC, Mehrdad wrote:

 AFAIT, that's impossible.
 
 Not because of D […]


 
 How else woud you implement things like parallel foreach, …?


 As I said, _ideally_ we wouldn't need opApply, but ranges fail 
 to be able to
 work in every use case that opApply does. As great as ranges 
 are, there _are_
 some cases where ranges are insufficient. So, we still have 
 opApply.


My comment was specifically meant in reply to Mehrdad's »Not 
because of D« statement.

David
Jul 16 2012
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Wednesday, July 11, 2012 14:10:35 Christophe Travert wrote:

 "monarch_dodra" , dans le message (digitalmars.D:171868), a =C3=A9cri=


t :

 I'm wondering if this is the correct behavior? In particular,
 since foreach guarantees a call to opSlice(), so writing "arr[]"
 *should* be redundant, yet the final behavior is different.
=20
 That said, the "issue" *could* be fixed if the base class defines
 opApply as: "return opSlice().opApply(dg)" (or more complex).
 However:
 a) The implementer of class has no obligation to do this, since
 he has provided a perfectly valid range.
 b) This would force implementers into more generic useless
 boilerplate code.
=20
 What are your thoughts? Which is the "correct" solution? Is it a
 bug with foreach, or should the base struct/class provide an
 opApply?


=20
 I think foreach should never call opSlice. That's not in the online
 documentation (http://dlang.org/statement.html#ForeachStatement), unl=


ess

 I missed something. If you want to use foreach on a class with an
 opSlice, then yes, you should define opApply. Otherwise, the user hav=


e

 to call opSlice himself, which seems reasonable. That's how I underst=


and

 the doc.


TDPL says that opSplice is called on a type passed to foreach if it def=
ines it.=20
Look at pages 380 - 381. The last paragraph of section 12.9.1 explicitl=
y=20
mentions it.

- Jonathna M Davis
Jul 11 2012
prev sibling parent reply kenji hara <k.hara.pg gmail.com> writes:
I think the online documentation
(http://dlang.org/statement.html#ForeachStatement) is not sufficient.

foreach (e; aggr) { ...body...}

Current dmd translates above foreach statement like follows.

1. If aggr has opApply or opApplyReverse, it's used.

2. If aggr has empty/front/popFront:
 2a. If aggr has slice operation, it's translated to:

  for (auto __r = aggr[];  // If aggr is a container (e.g. std.container.Array),
                           // foreach will get its range object for
the iteration.
      !__r.empty;
      __r.popFront()) { auto e = __r.front; ...body... }

 2b. If aggr doesn't have slice operation, it's translated to:

  for (auto __r = aggr;  // If aggr is copyable, saves the original range.
      !__r.empty;
      __r.popFront()) { auto e = __r.front; ...body... }

3. If aggr is static or dynamic array, it's translated to:

  for (auto __tmp = aggr[], __key = 0;  // If aggr is static array,
get its slice for iteration.
      !__key < __tmp.length;
      ++__key) { auto e = __tmp[__key]; ...body... }

These come from the dmd source code.
https://github.com/D-Programming-Language/dmd/blob/master/src/opover.c#L1226
https://github.com/D-Programming-Language/dmd/blob/master/src/statement.c#L1522

Bye

Kenji Hara

2012/7/11 monarch_dodra <monarch_dodra gmail.com>:

 If you create a class/struct that can give you a (forward) range via
 "opSlice()", and that range gives you access to "opApply", then you get two
 different behaviors:

 ----
 MyClass arr = ...;

 foreach(a; arr)
    ...

 foreach(a; arr[])
    ...

 ----
 In the first case, foreach will call opSlice(), and then walk through the
 resulting arr[] range with the front/popFront/empty troika.

 In the second case, foreach will call opApply on the range "arr[]".

 ----
 I'm wondering if this is the correct behavior? In particular, since foreach
 guarantees a call to opSlice(), so writing "arr[]" *should* be redundant,
 yet the final behavior is different.

 That said, the "issue" *could* be fixed if the base class defines opApply
 as: "return opSlice().opApply(dg)" (or more complex). However:
 a) The implementer of class has no obligation to do this, since he has
 provided a perfectly valid range.
 b) This would force implementers into more generic useless boilerplate code.

 What are your thoughts? Which is the "correct" solution? Is it a bug with
 foreach, or should the base struct/class provide an opApply?

 PS: Related: "foreach over range (with opApply) should save range."
 http://d.puremagic.com/issues/show_bug.cgi?id=4347
 On this assigned issue, is the conclusion that foreach will eventually save
 the range?
Jul 11 2012
parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Wed, 11 Jul 2012 11:10:16 -0400, kenji hara <k.hara.pg gmail.com> wrote:


 I think the online documentation
 (http://dlang.org/statement.html#ForeachStatement) is not sufficient.

 foreach (e; aggr) { ...body...}

 Current dmd translates above foreach statement like follows.

 1. If aggr has opApply or opApplyReverse, it's used.

 2. If aggr has empty/front/popFront:
  2a. If aggr has slice operation, it's translated to:

   for (auto __r = aggr[];  // If aggr is a container (e.g.  
 std.container.Array),
                            // foreach will get its range object for
 the iteration.
       !__r.empty;
       __r.popFront()) { auto e = __r.front; ...body... }

  2b. If aggr doesn't have slice operation, it's translated to:

   for (auto __r = aggr;  // If aggr is copyable, saves the original  
 range.
       !__r.empty;
       __r.popFront()) { auto e = __r.front; ...body... }

 3. If aggr is static or dynamic array, it's translated to:

   for (auto __tmp = aggr[], __key = 0;  // If aggr is static array,
 get its slice for iteration.
       !__key < __tmp.length;
       ++__key) { auto e = __tmp[__key]; ...body... }

 These come from the dmd source code.
 https://github.com/D-Programming-Language/dmd/blob/master/src/opover.c#L1226
 https://github.com/D-Programming-Language/dmd/blob/master/src/statement.c#L1522


This is wrong.  Why should we require aggr having empty/front/popFront to  
trigger a call to opSlice, which could have completely different type from  
aggr?  What if the result of opSlice has opApply?

If opSlice is to be used, this is how it should go (in order of  
precedence):

1. if aggr has opApply or opApplyReverse, use it.

2. if aggr has opSlice, and the result of aggr.opSlice() has opApply or  
opApplyReverse, use it.

3. if aggr has opSlice, and the result of aggr.opSlice() has  
empty/front/popfront, use it as in your 2a above.

4. if aggr has empty/front/popFront, use it as in your 2b above.

5. static or dynamic array.

I should also note that the existence of opApply should not preclude later  
possibilities if that opApply can't compile for the given foreach  
parameters.

-Steve
Jul 12 2012
parent reply "monarch_dodra" <monarch_dodra gmail.com> writes:
On Thursday, 12 July 2012 at 21:18:21 UTC, Steven Schveighoffer 
wrote:

 If opSlice is to be used, this is how it should go (in order of 
 precedence):

 1. if aggr has opApply or opApplyReverse, use it.

 2. if aggr has opSlice, and the result of aggr.opSlice() has 
 opApply or opApplyReverse, use it.

 3. if aggr has opSlice, and the result of aggr.opSlice() has 
 empty/front/popfront, use it as in your 2a above.

 4. if aggr has empty/front/popFront, use it as in your 2b above.

 5. static or dynamic array.

 I should also note that the existence of opApply should not 
 preclude later possibilities if that opApply can't compile for 
 the given foreach parameters.

 -Steve


4.1:Make copy first.
Jul 12 2012
parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Thu, 12 Jul 2012 17:35:23 -0400, monarch_dodra  
<monarch_dodra gmail.com> wrote:


 On Thursday, 12 July 2012 at 21:18:21 UTC, Steven Schveighoffer wrote:

 If opSlice is to be used, this is how it should go (in order of  
 precedence):

 1. if aggr has opApply or opApplyReverse, use it.

 2. if aggr has opSlice, and the result of aggr.opSlice() has opApply or  
 opApplyReverse, use it.

 3. if aggr has opSlice, and the result of aggr.opSlice() has  
 empty/front/popfront, use it as in your 2a above.

 4. if aggr has empty/front/popFront, use it as in your 2b above.

 5. static or dynamic array.

 I should also note that the existence of opApply should not preclude  
 later possibilities if that opApply can't compile for the given foreach  
 parameters.

 -Steve


 4.1:Make copy first.


Kenji's 2b does do that:


   for (auto __r = aggr;  // If aggr is copyable, saves the original range.
       !__r.empty;
       __r.popFront()) { auto e = __r.front; ...body... }


-Steve
Jul 12 2012
parent reply "monarch_dodra" <monarch_dodra gmail.com> writes:
On Thursday, 12 July 2012 at 22:02:19 UTC, Steven Schveighoffer 
wrote:

 On Thu, 12 Jul 2012 17:35:23 -0400, monarch_dodra 
 <monarch_dodra gmail.com> wrote:


 On Thursday, 12 July 2012 at 21:18:21 UTC, Steven 
 Schveighoffer wrote:

 If opSlice is to be used, this is how it should go (in order 
 of precedence):

 1. if aggr has opApply or opApplyReverse, use it.

 2. if aggr has opSlice, and the result of aggr.opSlice() has 
 opApply or opApplyReverse, use it.

 3. if aggr has opSlice, and the result of aggr.opSlice() has 
 empty/front/popfront, use it as in your 2a above.

 4. if aggr has empty/front/popFront, use it as in your 2b 
 above.

 5. static or dynamic array.

 I should also note that the existence of opApply should not 
 preclude later possibilities if that opApply can't compile 
 for the given foreach parameters.

 -Steve


 4.1:Make copy first.


 Kenji's 2b does do that:


   for (auto __r = aggr;  // If aggr is copyable, saves the 
 original range.
       !__r.empty;
       __r.popFront()) { auto e = __r.front; ...body... }


 -Steve


Oh, right "use it as in _your_ 2b above". Didn't get what you 
meant at first.

About the if "opApply can't compile": Do you mean:
a) If there is no matching opApply function?
b) Or if there is an actual error in the body of opApply?
I think you meant a) ?

I think your 1. and 2. should instead read:
1. if aggr has a matching opApply or opApplyReverse, use it.
2. if aggr has opSlice, and the result of aggr.opSlice() has a 
matching opApply or opApplyReverse, use it.
Jul 12 2012
parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Fri, 13 Jul 2012 02:20:45 -0400, monarch_dodra  
<monarch_dodra gmail.com> wrote:


 On Thursday, 12 July 2012 at 22:02:19 UTC, Steven Schveighoffer wrote:

 On Thu, 12 Jul 2012 17:35:23 -0400, monarch_dodra  
 <monarch_dodra gmail.com> wrote:


 On Thursday, 12 July 2012 at 21:18:21 UTC, Steven Schveighoffer wrote:

 If opSlice is to be used, this is how it should go (in order of  
 precedence):

 1. if aggr has opApply or opApplyReverse, use it.

 2. if aggr has opSlice, and the result of aggr.opSlice() has opApply  
 or opApplyReverse, use it.

 3. if aggr has opSlice, and the result of aggr.opSlice() has  
 empty/front/popfront, use it as in your 2a above.

 4. if aggr has empty/front/popFront, use it as in your 2b above.

 5. static or dynamic array.

 I should also note that the existence of opApply should not preclude  
 later possibilities if that opApply can't compile for the given  
 foreach parameters.

 -Steve


 4.1:Make copy first.


 Kenji's 2b does do that:


   for (auto __r = aggr;  // If aggr is copyable, saves the original  
 range.
       !__r.empty;
       __r.popFront()) { auto e = __r.front; ...body... }


 -Steve


 Oh, right "use it as in _your_ 2b above". Didn't get what you meant at  
 first.


hehe, sorry.  Should have said Kenji's...


 About the if "opApply can't compile": Do you mean:
 a) If there is no matching opApply function?
 b) Or if there is an actual error in the body of opApply?
 I think you meant a) ?

 I think your 1. and 2. should instead read:
 1. if aggr has a matching opApply or opApplyReverse, use it.
 2. if aggr has opSlice, and the result of aggr.opSlice() has a matching  
 opApply or opApplyReverse, use it.


Yes, I meant matching.  Meaning if I define front to return int, and  
opApply takes a delegate for a float, then foreach(int x; aggr) should use  
the range primitives, foreach(float x; aggr) should use opApply.

-Steve
Jul 13 2012