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

toInt doesn't raise an exception #2764

Closed
def- opened this Issue May 20, 2015 · 11 comments

Comments

Projects
None yet
6 participants
@def-
Copy link
Member

def- commented May 20, 2015

While the documentation states that toInt raises EInvalidValue if the conversion fails (because f is infinite for example), I can't get an exception:

echo toInt(Inf)

Prints:

-9223372036854775808

@Araq Araq added the Documentation label Jul 1, 2015

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Jul 1, 2015

I think we really like to inherit C's behaviour for these things.

@Varriount

This comment has been minimized.

Copy link
Contributor

Varriount commented Jul 1, 2015

Which is?

@dom96

This comment has been minimized.

Copy link
Member

dom96 commented Jul 2, 2015

@Varriount I'm guessing: the current behaviour.

narimiran added a commit to narimiran/Nim that referenced this issue Jan 3, 2019

@narimiran narimiran closed this in 77166ba Jan 4, 2019

narimiran added a commit that referenced this issue Jan 4, 2019

@timotheecour

This comment has been minimized.

Copy link
Contributor

timotheecour commented Jan 4, 2019

/cc @dom96 @Varriount @Araq
we could add an overload (but not in system.nim to avoid bloating) that raises OverflowError to be consistent with:

  • high(int)+1: Error: unhandled exception: over- or underflow [OverflowError]
  • toInt(a: float, static raises=false) => when raises==true, can raise OverflowError when calling toInt(1e19) instead of returning -9223372036854775808

@timotheecour timotheecour changed the title toInt doesn't raise an exception [TODO] toInt doesn't raise an exception Jan 4, 2019

@GULPF

This comment has been minimized.

Copy link
Member

GULPF commented Jan 4, 2019

Please dont add an overload, the default behavior should simply be changed. The issue is just that Nim doesnt do range checking for float to int conversions, and that should be fixed. There is an open issue for that somewhere.

@timotheecour

This comment has been minimized.

Copy link
Contributor

timotheecour commented Jan 4, 2019

ya id prefer safety by default as well. do u have a link to issue?

@dom96

This comment has been minimized.

Copy link
Member

dom96 commented Jan 4, 2019

@timotheecour what does the "[TODO]" mean? Isn't it the equivalent of reopening this issue?

@GULPF

This comment has been minimized.

Copy link
Member

GULPF commented Jan 4, 2019

@timotheecour Here's the issue for float range checking: #7179

@timotheecour

This comment has been minimized.

Copy link
Contributor

timotheecour commented Jan 4, 2019

@dom96 TODO means: there's something left to be done that is discussed in the thread

  • eg, for a PR (which doens't make sense to re-open once merged for eg)
  • eg, for an issue that was "mostly solved" where re-opening may be overkill (avoids fighting with whoever closed it, and avoids complaints from some ppl that care about minimizing number of opened issues for more minor issues) yet reminds that something is left to do. In some cases re-opening makes more sense, it's a gray area.

if #7179 now tracks this, I could remove the TODO from title, but doesn't seem like a dup

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Jan 5, 2019

TODO is just noise, nothing is ever perfect, so everything is TODO.

@Araq Araq changed the title [TODO] toInt doesn't raise an exception toInt doesn't raise an exception Jan 5, 2019

@timotheecour

This comment has been minimized.

Copy link
Contributor

timotheecour commented Jan 5, 2019

it's not noise, it helps triaging for whoever needs to revisit un-addressed points at a later time (esp the issue author or commenter).
The issue is marked as closed so doesn't affect open issue count or open issue dashboad, nor any person who just looks at opened issues.
Now that you've removed [TODO] from title, @GULPF 's point here #2764 (comment) is just lost ( #7179 doesnt' look like a dup)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment