Hi Joe,
Thanks a lot for taking the time to review !
Please see comments inlined.
On 23/09/2014 04:51, Joe Darcy wrote:
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.
Agreed.
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.
Sure. With this set of separate independent arrays, the test is
currently not much readable, and is weak with regards of maintenance
and evolutivity.
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 think so too. I even think it might be good to group HALF_EVEN with
those two, since behaviour of dtoa impacts
these 3 cases in the same manner when very close to tie.
Will change the code to group HALF_UP and HALF_DOWN.
Thanks,
Olivier.
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
