Hi Stuart,
Full webrev for review include including tests:
http://cr.openjdk.java.net/~darcy/8241374.0/
and the companion CSR:
https://bugs.openjdk.java.net/browse/JDK-8241805
Replies interspersed below.
On 3/28/2020 7:37 PM, Stuart Marks wrote:
Hi Joe,
Overall this looks quite good. Thanks for being thorough about this; I
certainly would have forgotten about StrictMath. :-)
I may have initial forgotten about StrictMath myself on at least one
prior occasion ;-)
Since this is integer arithmetic, is it true that the StrictMath
versions are identical to the Math versions?
Right; conceptually StrictMath is a subclass of Math where certain
methods have additional cross-platform reproducibility requirements.
However, since all the methods are static, at least the javadoc gets
replicated although the StrictMath implementations of integer methods
can just call the Math one. (In that case, the delegation could go the
other way too, but as Math is called more often, it is a little better
to have the real implementation hosted there.)
I have only a couple editorial quibbles.
+ * {@code int} range and an exception is thrown for that argument.
Recommend adding a comma here:
+ * {@code int} range, and an exception is thrown for that argument.
Changed to
{@code int range}, so an exception is thrown for that argument.
--
+ * @return the absolute value of the argument absent overflow
I stumbled while reading this. While not incorrect, it seems a bit
awkwardly worded to me. How about something like "...the argument,
unless overflow occurs."
Changed as suggested.
Normally the exception behavior is not described in an @return tag, but
the point of this method is its exceptional behavior so I think that
variance with typical practice is fine.
--
+ throw new ArithmeticException("Cannot represent " +
+ "absolute value of " +
+ "Integer.MIN_VALUE");
In cases like this I usually prefer to put the entire string on its
own line, avoiding the concatenation:
+ throw new ArithmeticException(
+ "Cannot represent absolute value of
Integer.MIN_VALUE");
Okay; also changed wording to:
"Overflow to represent absolute value of Integer.MIN_VALUE"
Thanks,
-Joe
