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 Issue 9126 - parse!int fails on size_t.max+1 digits long input (overflow) #996

Merged
merged 2 commits into from Dec 10, 2012

Conversation

NilsBossung
Copy link
Contributor

http://d.puremagic.com/issues/show_bug.cgi?id=9126
Could have seen this eleven months ago (#396) ...
I'm hesitating to add the test case, because it eats much time to test for a trivial bug.

The only purpose of those counters was to track if anything has been
parsed. A bool suffices and it doesn't overflow.
@alexrp
Copy link
Member

alexrp commented Dec 9, 2012

Well, we need some kind of test case to avoid a regression... why is adding one a problem?

@NilsBossung
Copy link
Contributor Author

The problem is, the running time of the tests explodes (from under 1 second to over 5 minutes on my machine).

@jmdavis
Copy link
Member

jmdavis commented Dec 10, 2012

dmd is extremely inefficient when dealing with large array literals, and regardless, if you really have an array of length size_t.max, the memory requirements are going to be quite large, and the GC is going to kill all performance, making testing such a thing essentially infeasible.

That being said, I'm not sure that there's much point in caring about arrays that long. Things go wrong in general when you reach arrays that large. Overflow is bound to happen on a calculation somewhere and screw you over. I would expect things to go wrong with arrays that large. However, if we have an easy fix that makes them less error-prone, I see nothing wrong with that.

jmdavis added a commit that referenced this pull request Dec 10, 2012
Fix Issue 9126 - parse!int fails on size_t.max+1 digits long input (overflow)
@jmdavis jmdavis merged commit dc39ebe into dlang:master Dec 10, 2012
@jmdavis
Copy link
Member

jmdavis commented Dec 10, 2012

Merged.

@braddr
Copy link
Member

braddr commented Dec 10, 2012

Wait.. why does it parse anywhere NEAR enough characters to take noticable time? The destination type overflows after so few characters that time shouldn't be a factor. Parsing past overflow of the destination is pretty pointless.

@jmdavis
Copy link
Member

jmdavis commented Dec 10, 2012

Wait.. why does it parse anywhere NEAR enough characters to take noticable time? The destination type overflows after so few characters that time shouldn't be a factor. Parsing past overflow of the destination is pretty pointless.

Good point. The fact that it doesn't implies that something else is going on here. As far as I can tell, these changes do indeed make it so that if you exceed size_t.max, it'll work properly, but I have no idea why it would ever reach that point if it's watching for overflow as it should.

@NilsBossung
Copy link
Contributor Author

Am Sonntag, 9. Dezember 2012, 18:26:45 schrieb Brad Roberts:

Wait.. why does it parse anywhere NEAR enough characters to take noticable
time? The destination type overflows after so few characters that time
shouldn't be a factor. Parsing past overflow of the destination is pretty
pointless.

Consider (a lot of) leading zeros. See the bug report for a test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants