Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
On Jan 14, 2014, at 8:56 PM, Brian Burkhalter brian.burkhal...@oracle.com wrote: Please see the updated webrev http://cr.openjdk.java.net/~bpb/8030814/webrev.3/. Hopefully my verbose description of the very clever overflow test is accurate and understandable. Also, I cleaned up the JTREG test a bit and added some tests based on the boundary values which split the range of the quantity guard. +1 a high comment to code ratio :-) I agree with Joe's comments. Paul.
Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
Given the identified items are changed accordingly is this approved? Thanks, Brian - Original Message - From: joe.da...@oracle.com To: brian.burkhal...@oracle.com Cc: core-libs-dev@openjdk.java.net Sent: Tuesday, January 14, 2014 7:28:11 PM GMT -08:00 US/Canada Pacific Subject: Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input Hi Brian, Lots good other than a few quibbles: We usually use /* * */ for long multi-line comments rather than // // // In the test update, we don't usually include mention of the bug id other than the @bug line. Thanks, -Joe On 1/14/2014 11:56 AM, Brian Burkhalter wrote: Please see the updated webrev http://cr.openjdk.java.net/~bpb/8030814/webrev.3/. Hopefully my verbose description of the very clever overflow test is accurate and understandable. Also, I cleaned up the JTREG test a bit and added some tests based on the boundary values which split the range of the quantity guard. Thanks, Brian
Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
Yes :-) -Joe On 01/15/2014 08:04 AM, Brian Burkhalter wrote: Given the identified items are changed accordingly is this approved? Thanks, Brian - Original Message - From: joe.da...@oracle.com To: brian.burkhal...@oracle.com Cc: core-libs-dev@openjdk.java.net Sent: Tuesday, January 14, 2014 7:28:11 PM GMT -08:00 US/Canada Pacific Subject: Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input Hi Brian, Lots good other than a few quibbles: We usually use /* * */ for long multi-line comments rather than // // // In the test update, we don't usually include mention of the bug id other than the @bug line. Thanks, -Joe On 1/14/2014 11:56 AM, Brian Burkhalter wrote: Please see the updated webrev http://cr.openjdk.java.net/~bpb/8030814/webrev.3/. Hopefully my verbose description of the very clever overflow test is accurate and understandable. Also, I cleaned up the JTREG test a bit and added some tests based on the boundary values which split the range of the quantity guard. Thanks, Brian
Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
Please see the updated webrev http://cr.openjdk.java.net/~bpb/8030814/webrev.3/. Hopefully my verbose description of the very clever overflow test is accurate and understandable. Also, I cleaned up the JTREG test a bit and added some tests based on the boundary values which split the range of the quantity guard. Thanks, Brian
Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
Hi Brian, Lots good other than a few quibbles: We usually use /* * */ for long multi-line comments rather than // // // In the test update, we don't usually include mention of the bug id other than the @bug line. Thanks, -Joe On 1/14/2014 11:56 AM, Brian Burkhalter wrote: Please see the updated webrev http://cr.openjdk.java.net/~bpb/8030814/webrev.3/. Hopefully my verbose description of the very clever overflow test is accurate and understandable. Also, I cleaned up the JTREG test a bit and added some tests based on the boundary values which split the range of the quantity guard. Thanks, Brian
Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
Hi Brian, This generally looks good, rather clever trick, i prefer it to using a cache. I agree with Joe some comments are required as it is not immediately obvious how this works. The additional tests seem adequate (overflow of the overflow), it's easy to go overboard e.g. you could test: BigInteger b = quotient.multiply(BigInteger.valueOf(radix + N)); for some range of N according to the radix under test. However, it might be useful to accurately test boundary conditions. Paul. On Jan 3, 2014, at 8:00 PM, Brian Burkhalter brian.burkhal...@oracle.com wrote: Issue:https://bugs.openjdk.java.net/browse/JDK-8030814 webrev: http://cr.openjdk.java.net/~bpb/8030814/webrev.2/ This review request follows from the discussion of last month in this thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024031.html The contributed patch before my minor tweaking of it is here http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024110.html with a detailed explanation of its logic here http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024136.html I added to the java/lang/Long/Unsigned JTREG test the case from the issue report as well as some other cases which exercise both sides of the A v B overflow test. Thanks, Brian
Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
On Jan 9, 2014, at 1:48 AM, Paul Sandoz wrote: This generally looks good, rather clever trick, i prefer it to using a cache. I agree with Joe some comments are required as it is not immediately obvious how this works. I have some simpler comments almost done being redacted. The additional tests seem adequate (overflow of the overflow), it's easy to go overboard e.g. you could test: BigInteger b = quotient.multiply(BigInteger.valueOf(radix + N)); for some range of N according to the radix under test. However, it might be useful to accurately test boundary conditions. Working on that also. Thanks, Brian
JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
Issue: https://bugs.openjdk.java.net/browse/JDK-8030814 webrev: http://cr.openjdk.java.net/~bpb/8030814/webrev.2/ This review request follows from the discussion of last month in this thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024031.html The contributed patch before my minor tweaking of it is here http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024110.html with a detailed explanation of its logic here http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024136.html I added to the java/lang/Long/Unsigned JTREG test the case from the issue report as well as some other cases which exercise both sides of the A v B overflow test. Thanks, Brian
Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
Hi Brian, I think more explanation of how the new code works needs to be included in the code (and the commented out code should be deleted). For the test, I would expect something with a bit simpler structure, but perhaps I don't fully understand the boundary cases of the new code. -Joe On 01/03/2014 11:00 AM, Brian Burkhalter wrote: Issue: https://bugs.openjdk.java.net/browse/JDK-8030814 webrev: http://cr.openjdk.java.net/~bpb/8030814/webrev.2/ This review request follows from the discussion of last month in this thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024031.html The contributed patch before my minor tweaking of it is here http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024110.html with a detailed explanation of its logic here http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024136.html I added to the java/lang/Long/Unsigned JTREG test the case from the issue report as well as some other cases which exercise both sides of the A v B overflow test. Thanks, Brian
