Hello,
Do the additional cases in the regression tests full cover the proposed
revision of the code changes?
Thanks,
-Joe
On 9/20/2014 4:06 PM, Sandipan Razzaque wrote:
Hi Brian -
Thanks for your review!
I think your point about adding !expOverflow to that conditional makes
perfect sense. We're only looking to account for expVal > expLimit where
decExp would be adjusted downward. Please adjust as appropriate.
Cheers,
SR
Sandipan Razzaque | www.sandipan.net
On Fri, Sep 19, 2014 at 4:54 PM, Brian Burkhalter <
[email protected]> wrote:
Hello Sandipan,
Finally got this off the back burner …
This review request follows this thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-June/027086.html
in which you provided a patch (thank you!) for:
https://bugs.openjdk.java.net/browse/JDK-8043740
I’ve created an updated webrev here:
http://cr.openjdk.java.net/~bpb/8043740/webrev.00/
Aside from minor reformatting there is no update to the proposed
FloatingDecimal change. I have not however included the test class
Bug8043740 from the contributed patch opting instead to update the existing
ParseDouble test by adding a few more strings to the goodStrings array.
The changes to FloatingDecimal appear reasonable to me. I am wondering
however if lines 2001-2002 should not be changed to include !expOverflow in
the conditional:
2001 if (!expOverflow && expSign == 1 && decExp < 0
2002 && (expVal + decExp) < expLimit) {
2003 // Cannot overflow: adding a positive and negative
number.
2004 decExp += expVal;
I don’t think that it’s possible for both expOverflow and the conditionals
at lines 2001-2002 of the webrev to all be true, but the additional test
would guarantee branching to the correct block.
Thanks,
Brian
On Jun 2, 2014, at 6:08 AM, Sandipan Razzaque <[email protected]> wrote:
I've made a quick revision to that last patch. Please find inline the
latest link + patch.
http://www.sandipan.net/public/webrevs/8043740/webrev.01/
