www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Is this a bug in iota?

reply "Brad Anderson" <eco gnuk.net> writes:
The following code:

     import std.range;
     void main()
     {
         auto i = iota(3);
         writeln(i.front, ", length: ", i.length); i.popFront();
         writeln(i.front, ", length: ", i.length); i.popFront();
         writeln(i.front, ", length: ", i.length); i.popFront();
         writeln(i.front, ", length: ", i.length); i.popFront();
         writeln(i.front, ", length: ", i.length); i.popFront();
     }

Outputs:

     0, length: 3
     1, length: 2
     2, length: 1
     3, length: 0
     4, length: 4294967295

You can popFront() for as long as you want well passed the 
length. Obviously popping off the front of a zero length range 
isn't valid but I would have expected a range violation to occur 
rather than it to silently continuing the series with a wrapped 
around length.

I was actually looking for an infinite counting range when I 
stumbled across this. sequence() works but something even simpler 
like iota() would be better. I suspect, however, that this 
behavior was unintended.

Regards,
Brad Anderson
Apr 18 2012
next sibling parent reply "bearophile" <bearophileHUGS lycos.com> writes:
Brad Anderson:
 You can popFront() for as long as you want well passed the 
 length. Obviously popping off the front of a zero length range 
 isn't valid but I would have expected a range violation to 
 occur rather than it to silently continuing the series with a 
 wrapped around length.

I think it's a matter of design and it's a matter of having an alternative Phobos release that contains asserts too. Adding the test slows down something (iota) that must be as fast as possible. And currently asserts are removed from the compiled Phobos... Bye, bearophile
Apr 18 2012
next sibling parent =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
On 04/18/2012 08:45 PM, Brad Anderson wrote:
 On Thursday, 19 April 2012 at 03:37:00 UTC, bearophile wrote:
 Brad Anderson:
 You can popFront() for as long as you want well passed the length.
 Obviously popping off the front of a zero length range isn't valid
 but I would have expected a range violation to occur rather than it
 to silently continuing the series with a wrapped around length.



I agree. I think it is just an oversight in iota()'s general implementation because its floating point specialization does have an assert check in popFront(): import std.range; void main() { auto r = iota(1.0); // <-- note 'double' type r.popFront(); r.popFront(); // <-- assertion failure }
 I think it's a matter of design and it's a matter of having an
 alternative Phobos release that contains asserts too. Adding the test
 slows down something (iota) that must be as fast as possible. And
 currently asserts are removed from the compiled Phobos...

 Bye,
 bearophile

Since iota is a template doesn't that mean it's not in phobos.lib but rather generated and built into my application? I'm not compiling in release mode so I would think any bounds checking it had would remain (I haven't yet looked at the source to see if there are any checks). I can definitely see stripping any bounds checking from a release build, of course.

I think this warrants an enhancement request. Thank you. :)
 Doing this same thing to a slice of an array does throw a Range
 Violation exception in release (and asserts in debug).

 Regards,
 Brad Anderson

Ali
Apr 19 2012
prev sibling parent reply Somedude <lovelydear mailmetrash.com> writes:
Le 19/04/2012 05:36, bearophile a écrit :
 Brad Anderson:
 You can popFront() for as long as you want well passed the length.
 Obviously popping off the front of a zero length range isn't valid but
 I would have expected a range violation to occur rather than it to
 silently continuing the series with a wrapped around length.

I think it's a matter of design and it's a matter of having an alternative Phobos release that contains asserts too. Adding the test slows down something (iota) that must be as fast as possible. And currently asserts are removed from the compiled Phobos... Bye, bearophile

You've gotta be kidding. How can this NOT be a bug ? import std.range, std.stdio; void main() { auto r = iota(3); //writeln(isInfinite!r); assert(!isInfinite!(int[])); assert(isInfinite!(Repeat!(int))); //assert(isRandomAccessRange!i); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); } Returns: 0, length: 3 empty ? false 1, length: 2 empty ? false 2, length: 1 empty ? false 3, length: 0 empty ? true 4, length: 4294967295 empty ? false
Apr 19 2012
parent reply Somedude <lovelydear mailmetrash.com> writes:
Le 19/04/2012 10:07, Jonathan M Davis a écrit :
 Having an assertion may be desirable, but the bug is in the usage of iota, not 
 iota itself. At best, the assertion would help indicate that the caller has a 
 bug. It's exactly the same as doing something like
 
 for(size_t i = 3; cond; --i) {}
 
 It's basic integer arithmetic. If you subtract from the minimum value that the 
 integral type will hold, then its value will wrap around to the maximum. So, 
 while adding an assertion would be desirable, I don't see how this could be 
 considered a bug in iota.
 
 - Jonathan M Davis

I don't get it, for me iota has nothing to do with the problem, the problem is in the implementation of popfront(), which should check beforehand whether the range is empty, right ?
Apr 19 2012
parent Somedude <lovelydear mailmetrash.com> writes:
Le 19/04/2012 11:11, Jonathan M Davis a écrit :
 On Thursday, April 19, 2012 10:14:39 Somedude wrote:
 Le 19/04/2012 10:07, Jonathan M Davis a écrit :
 Having an assertion may be desirable, but the bug is in the usage of iota,
 not iota itself. At best, the assertion would help indicate that the
 caller has a bug. It's exactly the same as doing something like

 for(size_t i = 3; cond; --i) {}

 It's basic integer arithmetic. If you subtract from the minimum value that
 the integral type will hold, then its value will wrap around to the
 maximum. So, while adding an assertion would be desirable, I don't see
 how this could be considered a bug in iota.

 - Jonathan M Davis

I don't get it, for me iota has nothing to do with the problem, the problem is in the implementation of popfront(), which should check beforehand whether the range is empty, right ?

Maybe, maybe not. popFront _must_ succeed. It has three options if the range is empty: assert, throw an exception, or just assume that it's going to succeed and choke in whatever manner the implementation ends up choking if the range is empty when it tries to pop an element from an empty range. Very few ranges are going to throw exceptions from popFront, because that incures overhead, and really, it's a bug in the caller if they keep trying to pop elements from an empty range. So, throwing an exception really isn't the correct behavior. Asserting is an option, and since iota is templated, it should probably do that (asserting in non-templated code is more debatable, because it'll only be there if Phobos itself is compiled without -release rather than the program or library call it - in most cases, such an assertion would probably never end up being run, because using a release version of Phobos is the default). But it's not doing that right now. An enhancement request for such would be appropriate.

Oh that's right. Still I think it should be done for development, and I also think Phobos should ship in both versions, dev AND release. We shouldn't link against the release phobos when we compile without -release.
Apr 19 2012
prev sibling next sibling parent "Brad Anderson" <eco gnuk.net> writes:
On Thursday, 19 April 2012 at 03:37:00 UTC, bearophile wrote:
 Brad Anderson:
 You can popFront() for as long as you want well passed the 
 length. Obviously popping off the front of a zero length range 
 isn't valid but I would have expected a range violation to 
 occur rather than it to silently continuing the series with a 
 wrapped around length.

I think it's a matter of design and it's a matter of having an alternative Phobos release that contains asserts too. Adding the test slows down something (iota) that must be as fast as possible. And currently asserts are removed from the compiled Phobos... Bye, bearophile

Since iota is a template doesn't that mean it's not in phobos.lib but rather generated and built into my application? I'm not compiling in release mode so I would think any bounds checking it had would remain (I haven't yet looked at the source to see if there are any checks). I can definitely see stripping any bounds checking from a release build, of course. Doing this same thing to a slice of an array does throw a Range Violation exception in release (and asserts in debug). Regards, Brad Anderson
Apr 18 2012
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, April 19, 2012 05:45:54 Brad Anderson wrote:
 Doing this same thing to a slice of an array does throw a Range
 Violation exception in release (and asserts in debug).

That's impossible for a library to do. There is no way to version code on whether it's in release mode or not. Only the compiler controls that. So, you can use assertions, which will then be around in non-release mode but not release mode, but you can't do anything which will be around in non-release mode and not release mode. The closest thing is -debug, but that's _completely_ different. It just enables debug blocks. - Jonathan M Davis
Apr 18 2012
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, April 19, 2012 09:58:00 Somedude wrote:
 Le 19/04/2012 05:36, bearophile a =C3=A9crit :
 Brad Anderson:
 You can popFront() for as long as you want well passed the length.=



 Obviously popping off the front of a zero length range isn't valid=



 I would have expected a range violation to occur rather than it to=



 silently continuing the series with a wrapped around length.

I think it's a matter of design and it's a matter of having an alternative Phobos release that contains asserts too. Adding the te=


 slows down something (iota) that must be as fast as possible. And
 currently asserts are removed from the compiled Phobos...
=20
 Bye,
 bearophile

You've gotta be kidding. How can this NOT be a bug ? =20 import std.range, std.stdio; =20 void main() { auto r =3D iota(3); //writeln(isInfinite!r); assert(!isInfinite!(int[])); assert(isInfinite!(Repeat!(int))); //assert(isRandomAccessRange!i); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); writeln(r.front, ", length: ", r.length, " empty ? ", r.empty); r.popFront(); } =20 Returns: 0, length: 3 empty ? false 1, length: 2 empty ? false 2, length: 1 empty ? false 3, length: 0 empty ? true 4, length: 4294967295 empty ? false

Having an assertion may be desirable, but the bug is in the usage of io= ta, not=20 iota itself. At best, the assertion would help indicate that the caller= has a=20 bug. It's exactly the same as doing something like for(size_t i =3D 3; cond; --i) {} It's basic integer arithmetic. If you subtract from the minimum value t= hat the=20 integral type will hold, then its value will wrap around to the maximum= . So,=20 while adding an assertion would be desirable, I don't see how this coul= d be=20 considered a bug in iota. - Jonathan M Davis
Apr 19 2012
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, April 19, 2012 10:14:39 Somedude wrote:
 Le 19/04/2012 10:07, Jonathan M Davis a =C3=A9crit :
 Having an assertion may be desirable, but the bug is in the usage o=


 not iota itself. At best, the assertion would help indicate that th=


 caller has a bug. It's exactly the same as doing something like
=20
 for(size_t i =3D 3; cond; --i) {}
=20
 It's basic integer arithmetic. If you subtract from the minimum val=


 the integral type will hold, then its value will wrap around to the=


 maximum. So, while adding an assertion would be desirable, I don't =


 how this could be considered a bug in iota.
=20
 - Jonathan M Davis

I don't get it, for me iota has nothing to do with the problem, the problem is in the implementation of popfront(), which should check beforehand whether the range is empty, right ?

Maybe, maybe not. popFront _must_ succeed. It has three options if the = range=20 is empty: assert, throw an exception, or just assume that it's going to= =20 succeed and choke in whatever manner the implementation ends up choking= if the=20 range is empty when it tries to pop an element from an empty range. Very few ranges are going to throw exceptions from popFront, because th= at=20 incures overhead, and really, it's a bug in the caller if they keep try= ing to=20 pop elements from an empty range. So, throwing an exception really isn'= t the=20 correct behavior. Asserting is an option, and since iota is templated, it should probably= do=20 that (asserting in non-templated code is more debatable, because it'll = only be=20 there if Phobos itself is compiled without -release rather than the pro= gram or=20 library call it - in most cases, such an assertion would probably never= end up=20 being run, because using a release version of Phobos is the default). B= ut it's=20 not doing that right now. An enhancement request for such would be=20 appropriate. Currently, it's taking the third option of not checking, and so you get= this=20 problem. But the fact that the code is attempting to pop off an element= from an=20 empty range is a bug in the caller, not popFront. - Jonathan M Davis
Apr 19 2012
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Thu, 19 Apr 2012 04:07:00 -0400, Jonathan M Davis <jmdavisProg gmx.com>  
wrote:

 Having an assertion may be desirable, but the bug is in the usage of  
 iota, not
 iota itself.

Yes, and iota should detect that bug with an assert. No case can really be made that iota shouldn't be changed. Please file an enhancement request. -Steve
Apr 19 2012
prev sibling next sibling parent "SomeDude" <lovelydear mailmetrash.com> writes:
On Thursday, 19 April 2012 at 11:38:39 UTC, Steven Schveighoffer 
wrote:
 On Thu, 19 Apr 2012 04:07:00 -0400, Jonathan M Davis 
 <jmdavisProg gmx.com> wrote:

 Having an assertion may be desirable, but the bug is in the 
 usage of iota, not
 iota itself.

Yes, and iota should detect that bug with an assert. No case can really be made that iota shouldn't be changed. Please file an enhancement request. -Steve

http://d.puremagic.com/issues/show_bug.cgi?id=7944
Apr 19 2012
prev sibling parent "Brad Anderson" <eco gnuk.net> writes:
On Thursday, 19 April 2012 at 12:39:25 UTC, SomeDude wrote:
 On Thursday, 19 April 2012 at 11:38:39 UTC, Steven 
 Schveighoffer wrote:
 On Thu, 19 Apr 2012 04:07:00 -0400, Jonathan M Davis 
 <jmdavisProg gmx.com> wrote:

 Having an assertion may be desirable, but the bug is in the 
 usage of iota, not
 iota itself.

Yes, and iota should detect that bug with an assert. No case can really be made that iota shouldn't be changed. Please file an enhancement request. -Steve

http://d.puremagic.com/issues/show_bug.cgi?id=7944

https://github.com/D-Programming-Language/phobos/pull/545 Regards, Brad Anderson
Apr 19 2012