Hi Joe,
Thanks for the relevant feedback.
I am going to provide all these changes in a new webrev within a few hours.
Thanks,
Olivier.
On 07/10/2014 18:25, Joe Darcy wrote:
Hi Olivier,
Some rewording suggestions:
445 * @param alreadyRounded Boolean indicating if rounding up
already happened.
446 * @param allDecimalDigits Boolean indicating if the digits
provide an exact
447 * representation of the value.
Including the type of the parameter is redundant with its declaration
so I'd recommend something like
445 * @param alreadyRounded whether or not rounding up has
already happened.
Given the semantics of allDecimalDigits, I recommend renaming the
parameter to something like "valueExactAsDecimal".
For
please restore the conventional formatting
532 } else if (digits[maximumDigits] == '5') {
The code
543 if (roundingMode ==
RoundingMode.HALF_UP) {
544 // Strictly follow HALF_UP rule
==> round-up
545 return true;
546 }
547 else {
548 // Strictly follow HALF_DOWN rule
==> don't round-up
549 return false;
550 }
can be replaced by
545 return roundingMode ==
RoundingMode.HALF_UP;
with a suitable comment.
Please make these adjustments and I'll do a careful review of the
rounding logic.
Thanks,
-Joe
On 10/2/2014 9:29 AM, olivier.lagn...@oracle.com wrote:
Please review this 2nd version of the fix taking into account your
feedback.
Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.01
<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.01>
I have changed the code following your remarks
Testing: lot of testing on jtreg, jck8, code coverage, jprt.
Below are more details on the changes in this webrev.
For the new work in question, it might be clearer to me if the
HALF_UP and HALF_DOWN cases
were combined into a single block since they > share much of the logic.
The unique logic for each mode would be easier to see if the
differences were placed together.
I have merged HALF_UP and HALF_DOWN cases into a single switch case.
I also tested a fully merged version where HALF_UP, HALF_DOWN, and
also HALF_EVEN are merge
into a single switch case. So merging the three is even possible.
If you want to have the three HALF_* cases we can quickly move to that.
My only suggestions are trivial: 1) enhance the method javadoc of
shouldRoundUp(), e.g., there are no @param tags for the boolean
parameters,
Fixed that. @param tags are available for shouldRoundUp method
2) use braces around all the statements contained in if/else blocks
(see below). Comment #2 is nit-picky.
Took care to follow this rule in this new version.
If the test is already printing out the information you showed above
(“Error formatting …”) then I think it is enough but the verbiage
should perhaps match the reminder, e.g., “Failure: Error formatting
double …”
Changed test code to provide this consistent wording.
Thanks,
Olivier.
