Review: JDK 8 CR for Support Integer overflow
Thanks for the review and comments: The comments and suggestions are included in the updated webrev: http://cr.openjdk.java.net/~rriggs/6708398.1 * Corrected error in multipleExact(long,long) with the special case Long.MIN_VALUE * -1. * Verified that retaining the optimization for small (2^31) arguments is worthwhile, not doing the divide saves about 1/2 on the execution time. * Removed the negateExact methods since they don't pull their weight in the API, simple tests for MIN_VALUE and MAX_VALUE can be done by the developer more efficiently. * Simplified the arguments to the ArithmeticExceptions to be simple strings since debugging this kind of exception requires the source code. * Expanded the comments in the implementation include descriptions and references to the Hackers Delight where they are used. * Updated the tests to include missing test cases More comments, please Thanks, Roger
Looks good to me (emcmanus). One really trivial thing is that Math.java:830 is indented one space too far. I thought that the common (int)value subexpression could be put in a variable in toIntExact but it turns out that javac generates more code in that case. Éamonn On 6 February 2012 13:16, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Thanks for the review and comments:
The comments and suggestions are included in the updated webrev: http://cr.openjdk.java.net/~rriggs/6708398.1
* Corrected error in multipleExact(long,long) with the special case Long.MIN_VALUE * -1. * Verified that retaining the optimization for small (2^31) arguments is worthwhile, not doing the divide saves about 1/2 on the execution time. * Removed the negateExact methods since they don't pull their weight in the API, simple tests for MIN_VALUE and MAX_VALUE can be done by the developer more efficiently. * Simplified the arguments to the ArithmeticExceptions to be simple strings since debugging this kind of exception requires the source code. * Expanded the comments in the implementation include descriptions and references to the Hackers Delight where they are used. * Updated the tests to include missing test cases
More comments, please
Thanks, Roger
On 6 February 2012 21:16, Roger Riggs <Roger.Riggs@oracle.com> wrote:
* Removed the negateExact methods since they don't pull their weight in the API, simple tests for MIN_VALUE and MAX_VALUE can be done by the developer more efficiently.
Sorry, but I can't agree with this. Developers get negation of numbers wrong all the time by ignoring the special case. Its a big source of hidden bugs. Increment/decrement are replaceable by add/subtract (with less readability), but negate is not. The whole purpose of methods like this is not to say "oh they're easy for the caller to write", but to write it once accurately, well-tested, and with clear intent for code readers. Look at the methods on Objects to see that complexity of the implementation is not the deciding factor. Many, many developers will thank you for having negate, increment and decrement. Stephen
I'm with Roger on this.
Sorry, but I can't agree with this. Developers get negation of numbers wrong all the time by ignoring the special case. Its a big source of hidden bugs. Increment/decrement are replaceable by add/subtract (with less readability), but negate is not.
First of all, negate(x) certainly is replaceable by subtractExact(0, x). Secondly, the problem is more that people don't realize that -x might not be exact. If you know there's a problem then it isn't any easier for you to know that a solution is to be found in Math than to know that the problem is with MIN_VALUE. Éamonn On 6 February 2012 14:35, Stephen Colebourne <scolebourne@joda.org> wrote:
On 6 February 2012 21:16, Roger Riggs <Roger.Riggs@oracle.com> wrote:
* Removed the negateExact methods since they don't pull their weight in the API, simple tests for MIN_VALUE and MAX_VALUE can be done by the developer more efficiently.
Sorry, but I can't agree with this. Developers get negation of numbers wrong all the time by ignoring the special case. Its a big source of hidden bugs. Increment/decrement are replaceable by add/subtract (with less readability), but negate is not.
The whole purpose of methods like this is not to say "oh they're easy for the caller to write", but to write it once accurately, well-tested, and with clear intent for code readers. Look at the methods on Objects to see that complexity of the implementation is not the deciding factor.
Many, many developers will thank you for having negate, increment and decrement.
Stephen
participants (3)
-
Eamonn McManus
-
Roger Riggs
-
Stephen Colebourne