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

MPR#7390: fix the caml_float_of_hex function #1528

Merged
merged 2 commits into from Dec 19, 2017

Conversation

Projects
None yet
3 participants
@oandrieu
Copy link
Contributor

oandrieu commented Dec 13, 2017

  • make sure the adjusted exponent does not overflow
  • return 0/inf instead of raising an exception for exponent values ≤ INT_MIN or ≥ INT_MAX

This fixes the first two points of the MPR.

MPR#7390: fix the caml_float_of_hex function
- make sure the adjusted exponent does not overflow
- return 0/inf instead of raising an exception for exponent values
  ≤INT_MIN or ≥INT_MAX.
@xavierleroy
Copy link
Contributor

xavierleroy left a comment

Thanks for the code. This looks very good to me. Maybe we could have an extra review from someone who likes arithmetic. @murmour would you like to have a look?

else if (e >= INT_MAX) {
*res = m == 0 ? 0. : HUGE_VAL;
return 0;
}

This comment has been minimized.

@murmour

murmour Dec 15, 2017

Contributor

This change is questionable.

MPR states:

The test "if (e < INT_MIN || e > INT_MAX) return -1;" is ineffective on Win64 since int and long have the same size there: this results in some difference in behavior between win64 and linux.

What is the difference? Previously, the conversion failed when e did not fit into an int, regardless of whether int and long have the same width (as if we used an imaginary strtoi function); but now the behaviour depends on the size of long.

Also, INT_MIN and INT_MAX are now special cases. Why were < and > changed into <= and >=?

This comment has been minimized.

@xavierleroy

xavierleroy Dec 15, 2017

Contributor

I think I agree with the change. There are two cases where the exponent overflows:

  • strtol overflows because the exponent is not representable as a long, in which case it returns LONG_MAX or LONG_MIN.
  • the long returned by strtol cannot be represented as an int.

The old test e < INT_MIN || e > INT_MAX catches both issues at once but only if LONG_MIN < INT_MIN and LONG_MAX > INT_MAX, which is not the case under Windows.

The new tests e <= INT_MIN and e >= INT_MAX catches both issues even if LONG_MIN == INT_MIN and LONG_MAX == INT_MAX as in Windows.

Treating e == INT_MIN as an underflow and e == INT_MAX as an overflow is OK floating-point wise.

This comment has been minimized.

@murmour

murmour Dec 15, 2017

Contributor

Got it: strtol overflow was not detected on Windows. It makes sense now. Thank you!

The change looks correct as it is, but perhaps it'd be better to check errno, if only for clarity?

errno = 0;
e = strtol(s, &p, 10);
/* out of range of type long? */
if (errno == ERANGE && (e == LONG_MIN || e == LONG_MAX)) return -1;
/* out of range of type int? */
if (e < INT_MIN || e > INT_MAX) return -1;
exp = (int)e;

This comment has been minimized.

@murmour

murmour Dec 15, 2017

Contributor

This check in the code above appears redundant: e == LONG_MIN || e == LONG_MAX. I took it from CERT C.

This comment has been minimized.

@xavierleroy

xavierleroy Dec 16, 2017

Contributor

Yes, we could follow the pattern of tests from the CERT example, because it is a lot clearer! But note that return -1 is no longer the correct action. I'm torn about efficiency: string->float conversion may be time-critical and the CERT code performs more tests than @oandrieu's code. On the other hand calling strtod and ldexp isn't cheap already.

This comment has been minimized.

@murmour

murmour Dec 16, 2017

Contributor

An explicit #ifdef could be even better, for both clarity and performance:

#if INT_MAX == LONG_MAX
      if (errno == ERANGE) return -1;
#else
      if (e < INT_MIN || e > INT_MAX) return -1;
#endif

This should make it more obvious to the reader that int can have the same width as long, and that it affects error-checking.

This comment has been minimized.

@oandrieu

oandrieu Dec 18, 2017

Author Contributor

But is there a need to make this function fail when the exponent overflows ? I don't see the point. What's wrong with returning 0 / ∞ ?

Personally, I prefer a version that doesn't use the preprocessor nor a global value.
I can add comments to explain that the test is laid out this way because of Win64.

This comment has been minimized.

@murmour

murmour Dec 18, 2017

Contributor

But is there a need to make this function fail when the exponent overflows ? I don't see the point. What's wrong with returning 0 / ∞ ?

Nothing wrong. I think it's a slight improvement, and was mostly confused by the error detection logic, which I found totally unobvious at first sight.

I can add comments to explain that the test is laid out this way because of Win64.

Another fine option, yes.

This comment has been minimized.

@oandrieu

oandrieu Dec 18, 2017

Author Contributor

OK I added a comment.

This comment has been minimized.

@murmour

murmour Dec 18, 2017

Contributor

The comment brings clarity. Thank you!

exp = INT_MIN;
else
exp = exp + adj;
}

This comment has been minimized.

@murmour

murmour Dec 15, 2017

Contributor

The overflow detection part seems correct.

This comment has been minimized.

@murmour

murmour Dec 15, 2017

Contributor

A readability+performance suggestion: you can add a caml_iadd_overflow function to misc.h and make use of it here.

This comment has been minimized.

@oandrieu

oandrieu Dec 18, 2017

Author Contributor

Hmm the functions there have a different semantic (modulo operation). Even with the GCC builtin there will be tests and branches so I'm not sure there's much gain in performance.

This comment has been minimized.

@murmour

murmour Dec 18, 2017

Contributor

Oh, sure: it's saturated arithmetic. Sorry. Looks good enough as it is.

add some comment
point out that the inclusive test for INT_MIN/INT_MAX is
intentional because of Win64.
@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 19, 2017

The comment is minimalistic but it helps. CI passes. Merging!

@xavierleroy xavierleroy merged commit 2cc67bb into ocaml:trunk Dec 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fdopen added a commit to fdopen/ocaml-ctypes that referenced this pull request Jun 24, 2018

windows: fix tests for OCaml 4.07
workarounds for
 - ocaml/ocaml#1535 (no dll dependency)
 - ocaml/ocaml#1528 (string_of_float format)
 - ocaml/ocaml#1479 (chdir changes environment)

fdopen added a commit to fdopen/ocaml-ctypes that referenced this pull request Dec 6, 2018

windows: fix issues introduced by OCaml 4.07
workarounds for
 - ocaml/ocaml#1535 (no dll dependency)
 - ocaml/ocaml#1528 (string_of_float format)
 - ocaml/ocaml#1479 (chdir changes environment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.