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 < brian.burkhal...@oracle.com> 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 <m...@sandipan.net> 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/ > > > >
