Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 14832: iota should have size_t length. #3571

Closed
wants to merge 2 commits into from

Conversation

quickfur
Copy link
Member

On 32-bit, iota(10UL) produces a range whose length is not convertible to size_t, causing array() to fail, even though its actual length is well within the range of size_t.

This fix makes iota always return size_t as the length. It will throw an exception if the length is not representable as size_t.

@quickfur
Copy link
Member Author

This PR is an alternative to #3544 which was rejected.

@DmitryOlshansky
Copy link
Member

I do not believe that pulling in the whole mess of std.conv is the right choice here

@WalterBright
Copy link
Member

As in #3544, I still do not understand the purpose behind allocating arrays of ulong length in 32 bit code. I'd immediately red flag any code that tried to do that as highly suspicious. Just because it can be made to work does not mean it should be.

@WalterBright
Copy link
Member

I strongly oppose this absent a compelling rationale for it.

@quickfur
Copy link
Member Author

@WalterBright I think there's some miscommunication here. The original bug report is not talking about allocating arrays of ulong length, it's talking about allocating an array of ulong elements returned by iota. It just so happens that iota's implementation has a length member that's ulong, which is arguably the wrong thing to do.

In the original PR, the possibility of fixing iota to only have a size_t length was raised, but the objection was that an indexed foreach loop over the result would have problems with 32-bit size_t overflow.

@quickfur
Copy link
Member Author

@DmitryOlshansky Would changing this to an explicit comparison with size_t.max be acceptable?

@DmitryOlshansky
Copy link
Member

@quickfur - test + throw error is enough. Then it stays nothrow at least.

Allocating 2^^32 and beyond in 32-bit is a programming error. Either RangeError or AssertFailure

@DmitryOlshansky
Copy link
Member

I strongly oppose this absent a compelling rationale for it.

iota(10UL).array - there are other cases with bigger numbers.

@jmdavis
Copy link
Member

jmdavis commented Aug 23, 2015

In the original PR, the possibility of fixing iota to only have a size_t length was raised, but the objection was that an indexed foreach loop over the result would have problems with 32-bit size_t overflow.

Any range that has length other than size_t is just plain broken. iota needs to be fixed, not array. No range should have a length member that is anything other than size_t.

@quickfur
Copy link
Member Author

Did what @jmdavis proposed. It works, but dang, it makes the code so ugly. And something just doesn't feel right about having both length and rawLength. I'm pretty sure this will get shot down too, but whatever.

@quickfur quickfur changed the title Fix 14832: array() should be able to make array of ulong length. Fix 14832: iota should have size_t length. Aug 24, 2015
@jmdavis
Copy link
Member

jmdavis commented Aug 24, 2015

iota is an abnormal case. In almost no other cases would supporting lengths other than size_t even come up (except where someone unthinkingly just uses int or long). And having iota support anything for length other than size_t just makes life nasty everywhere else (either because iota doesn't work with a bunch of stuff or because a bunch of other stuff has to be tweaked just to support iota, making the problem propagate). So, while I agree that it's not nice to make iota uglier, if that's what it takes for iota to support long or ulong on 32-bit systems (or ulong on 64-bit systems for that matter, since ulong holds larger values than size_t can even on 64-bit machines) while having its length be size_t like every other range, then so be it.

And actually, looking at hasLength, for some reason, it requires

 ulong l = r.length;

rather than

size_t l = r.length;

I'm guessing that someone did that because of iota, but it should really be changed to size_t. iota is the only case where we've supported anything other than size_t for length, and we've been talking about how that was a bad idea ever since that change was made. Supporting anything else just propagates nastily, causing problems all over the place, especially once arrays get involved, since their length is size_t (which is why length for everything else is supposed to be size_t).

@quickfur
Copy link
Member Author

I thought about this a bit more. While iota may at first glance be the only exception to length always being size_t, I can actually think of many cases where a finite range may have a computable but not representable length. For example, anything involving combinatorics, like a cartesianProduct of ranges of finite length, can easily overflow size_t, especially on 32-bit systems. To use a contrived example, a range that returns the ith element of a Goodstein sequence will also easily overflow size_t, even though you could validly ask for the last n elements.

So it appears that there are indeed ranges out there that may have finite length that cannot be represented by size_t. So iota wouldn't be the first case of such ranges.

The question then is, should such ranges still have a length member? If so, what should they do when the length cannot be represented by size_t? Throw?

@DmitryOlshansky
Copy link
Member

The question then is, should such ranges still have a length member? If so, what should they do when the length cannot be represented by size_t? Throw?

These abnormal ranges might work with front/popFront/empty but the length should then check for overflow and throw error I guess. Overflow can be checked with these nice core.checkedint thingies.

@schveiguy
Copy link
Member

I can actually think of many cases

A file comes to mind as well.

In almost no other cases would supporting lengths other than size_t even come up

There isn't a good rationale to support this requirement. Basically, the only reason to require length to be size_t is for array. I can't think of another valid reason.

A common-sense implementation for hasLength is:

  1. Can I get the length via r.length?
  2. Is the length an integer type?

The third requirement of "Will it fit into the local architecture's word size" doesn't seem to jive with portability, or D's approach to types. It seems very arbitrary and unnecessary.

Objectively, I don't think the change is correct. If you want to disallow iotas with larger than size_t length, it should be done on creation, not on a call to length. The current PR now makes code that diligently tests for length and optimizes based on that, compile and fail at runtime, where there is nothing wrong with that code. The code in error is iota (if we decide that's the position we want to take).

Unobjectively, I don't agree with the change to iota at all. The only culprit causing misery here is array (and only when called on 32-bit systems with ranges that have ulong length), it should get the treatment.

@schveiguy
Copy link
Member

The third requirement of "Will it fit into the local architecture's word size" doesn't seem to jive with portability, or D's approach to types

OR current code that currently compiles and works as expected (regardless of what we think of its utility).

@quickfur
Copy link
Member Author

Wow, this is so ridiculous. A trivial issue like creating an array of ulongs from iota can get into this endless debate about what length should or should not be or which code should be responsible for handling what. I regret ever getting involved with this. Obviously no solution will be acceptable to everyone, so I'm calling it quits. Let someone else foolhardy enough to try to fix this step up.

@quickfur quickfur closed this Aug 24, 2015
@quickfur quickfur deleted the issue14832 branch August 24, 2015 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants