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

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.

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 check in the code above appears redundant: e == LONG_MIN || e == LONG_MAX. I took it from CERT C.

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.

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.

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.

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.

OK I added a comment.

The comment brings clarity. Thank you!

The overflow detection part seems correct.

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

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.

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