Thanks Joe,
Will modify and check the tests accordingly.
Brian, I guess you are still ok with these code changes ?
Thanks a lot both of you for the reviewing.
Thanks a lot William for checking with your own tests. That is very much
appreciated ! ;-)
Olivier.
On 09/10/2014 19:16, Joe Darcy wrote:
Hi Olivier,
Please change the formatting any if-else statements like
537 }
538 else {
545 }
546 else {
552 }
553 else {
to be
537 } else {
More substantively, these lines
547 // Not an exact binary representation.
548 if (alreadyRounded) {
549 // Digit sequence rounded-up. Was
below tie.
550 // Don't round-up twice !
551 return false;
552 }
553 else {
554 // Digit sequence truncated. Was
above tie.
555 // must round-up !
556 return true;
557 }
Can be replaced by
return !alreadyRounded;
with appropriate commenting.
Please make these changes; as long as the tests still pass, no
re-review is needed.
Thanks,
-Joe
On 10/9/2014 3:27 AM, olivier.lagn...@oracle.com wrote:
Please review this 3rd version of the fix taking into account latest
feedback from Joe.
Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
Webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.02/
<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.02/>
List of changes from previous webrev:
- renamed "allDecimalDigits" to "valueExactAsDecimal"
- changed related @param javadoc comments
- restored conventional formatting on "else if" clause
- changed rounding decision code when last digit at rounding position
and valueExactAsDecimal true to synthetic return statement:
"return roundingMode == RoundingMode.HALF_UP;" with adapted comment.
testing: jtreg, jck8, jprt, code coverage.
Thanks for reviewing,
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.
