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
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 reply "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
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 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 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=






 but

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







 silently continuing the series with a wrapped around length.


=20
 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=




st

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


=20
 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
next sibling 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 reply 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=




f iota,

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




e

 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=




ue 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.
=20
 - Jonathan M Davis


=20
 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
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 parent reply "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
parent reply "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
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