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
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 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 next sibling parent "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
prev sibling next sibling parent 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
prev sibling next 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=

 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?

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

 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=

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

 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 next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling 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
prev sibling next sibling parent 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
prev sibling next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling parent 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

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

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