Hello,
I've looked over the proposed changeset as well.
I don't see a problem with the code, but I'm not (yet) convinced it is
right.
For future work, I think be clearer to combine the CEILING and FLOOR
cases to share a loop with a condition test based on CEILING/FLOOR lie.
In the test, again for future work, I think it would be clearer to
create a custom class to represent the tuple of information needed for a
test case as opposed to spreading that information about over a set of
parallel arrays.
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.
Thanks,
-Joe
On 09/22/2014 08:50 AM, Brian Burkhalter wrote:
Hi Olivier,
On Sep 22, 2014, at 7:56 AM, olivier.lagn...@oracle.com wrote:
and 2) use braces around all the statements contained in if/else blocks (see
below). Comment #2 is nit-picky.
I tried to keep the same flavour of writing as in HALF_DOWN and HALF_EVEN
cases, i.e. don't use brace for
terminal/leaf return true/false statements. This is not the standard however,
at least in this file.
Will use braces in all case (i.e. the 3 of HALF_UP, HALF_DOWN and HALF_EVEN).
I did not look at the other case. If your formatting matches the rest of the
file then I think it is OK to leave it as-is.
Lastly and this is not really part of your changeset, but I found it curious
that the test prints the details of all cases that succeed as opposed to those
that fail. I would think that either all results or the failures alone ought to
be printed instead of successes only. See for example the partial diff below
the DigitList diff.
Since these are most often corner and tricky test cases I think it interesting
to have the details of each result,
including infos of both why returned result is correct or wrong.
That can help the reader to understand all these tricky cases.
The bad side of it being that it prints a lot of text, with failure cases
(hoepfully few) lost in the middle of it,
thus making failures possibly not immediate to detect.
Here is an example of what is printed in case of failure:
"
========================================
***Error formatting double value from string : 0.6868d
NumberFormat pattern is : #,##0.###
Maximum number of fractional digits : 3
Fractional rounding digit : 4
Position of value relative to tie : above
Rounding Mode : HALF_UP
BigDecimal output : 0.68679999999999996607158436745521612465381622314453125
FloatingDecimal output : 0.6868
Error. Formatted result different from expected.
Expected output is : "0.687"
Formated output is : "0.686"
========================================
I missed that output: I was looking for the word “failure.”
There is also a reminder of the number of errors at the end of the report:
"
==> 4 tests failed
Test failed with 4 formating error(s).
"
May be providing a reminder (value + rounding-mode + rounding position)
of the failure cases at the end of the would be better ?
Like:
"
Test failed with 4 formating error(s) for following cases :
- 0.3126d, HALF_UP rounding, 3 fractional digits
- 0.6868d, HALF_UP rounding, 3 fractional digits
- 1.798876d, HALF_UP rounding, 5 fractional digits
- 1.796889d, HALF_UP rounding, 5 fractional digits
"
Would doing so be ok ?
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 …”
Thanks,
Brian
