Re: RFR JDK-8166261: Scanner.nextInt(int) (and similar methods) throws PatternSyntaxException
+1 Naoto On 10/7/16 4:08 PM, Xueming Shen wrote: On 10/07/2016 03:52 PM, Naoto Sato wrote: The test case now looks good. Using biconsumer makes it more readable. I see the additional paragraph with regard to IllegalArgumentException in each method description, however, it contradicts the @throws clause. Need to add @throws IllegalArgumentException for each method? thanks. updated. somehow I messed up with two versions there. Naoto On 10/7/16 2:38 PM, Xueming Shen wrote: thanks! updated, with biconsumer as well. http://cr.openjdk.java.net/~sherman/8166261/webrev/ On 10/05/2016 10:25 AM, Naoto Sato wrote: Looks good to me. The test case could use IntStream.rangeClosed(Character.MIN_RADIX, Character.MAX_RADIX) for the good radixes, instead of hard coding ints. Naoto On 10/5/16 8:53 AM, Xueming Shen wrote: Hi Please help review issue: https://bugs.openjdk.java.net/browse/JDK-8166261 webre: http://cr.openjdk.java.net/~sherman/8166261/webrev The radix sanity check are missing from hasNextByte/Short/Int/Long/BigInteger(). The only method we are doing now is useRadix(). The proposed change here is to add the check into all above methods that take a radix. Arguably it's an incompatible api change, but I don't expect it really breaks anyone's code. Need go through ccc if approved. Thanks, Sherman
Re: RFR JDK-8166261: Scanner.nextInt(int) (and similar methods) throws PatternSyntaxException
On 10/07/2016 03:52 PM, Naoto Sato wrote: The test case now looks good. Using biconsumer makes it more readable. I see the additional paragraph with regard to IllegalArgumentException in each method description, however, it contradicts the @throws clause. Need to add @throws IllegalArgumentException for each method? thanks. updated. somehow I messed up with two versions there. Naoto On 10/7/16 2:38 PM, Xueming Shen wrote: thanks! updated, with biconsumer as well. http://cr.openjdk.java.net/~sherman/8166261/webrev/ On 10/05/2016 10:25 AM, Naoto Sato wrote: Looks good to me. The test case could use IntStream.rangeClosed(Character.MIN_RADIX, Character.MAX_RADIX) for the good radixes, instead of hard coding ints. Naoto On 10/5/16 8:53 AM, Xueming Shen wrote: Hi Please help review issue: https://bugs.openjdk.java.net/browse/JDK-8166261 webre: http://cr.openjdk.java.net/~sherman/8166261/webrev The radix sanity check are missing from hasNextByte/Short/Int/Long/BigInteger(). The only method we are doing now is useRadix(). The proposed change here is to add the check into all above methods that take a radix. Arguably it's an incompatible api change, but I don't expect it really breaks anyone's code. Need go through ccc if approved. Thanks, Sherman
Re: RFR JDK-8166261: Scanner.nextInt(int) (and similar methods) throws PatternSyntaxException
The test case now looks good. Using biconsumer makes it more readable. I see the additional paragraph with regard to IllegalArgumentException in each method description, however, it contradicts the @throws clause. Need to add @throws IllegalArgumentException for each method? Naoto On 10/7/16 2:38 PM, Xueming Shen wrote: thanks! updated, with biconsumer as well. http://cr.openjdk.java.net/~sherman/8166261/webrev/ On 10/05/2016 10:25 AM, Naoto Sato wrote: Looks good to me. The test case could use IntStream.rangeClosed(Character.MIN_RADIX, Character.MAX_RADIX) for the good radixes, instead of hard coding ints. Naoto On 10/5/16 8:53 AM, Xueming Shen wrote: Hi Please help review issue: https://bugs.openjdk.java.net/browse/JDK-8166261 webre: http://cr.openjdk.java.net/~sherman/8166261/webrev The radix sanity check are missing from hasNextByte/Short/Int/Long/BigInteger(). The only method we are doing now is useRadix(). The proposed change here is to add the check into all above methods that take a radix. Arguably it's an incompatible api change, but I don't expect it really breaks anyone's code. Need go through ccc if approved. Thanks, Sherman
Re: RFR JDK-8166261: Scanner.nextInt(int) (and similar methods) throws PatternSyntaxException
thanks! updated, with biconsumer as well. http://cr.openjdk.java.net/~sherman/8166261/webrev/ On 10/05/2016 10:25 AM, Naoto Sato wrote: Looks good to me. The test case could use IntStream.rangeClosed(Character.MIN_RADIX, Character.MAX_RADIX) for the good radixes, instead of hard coding ints. Naoto On 10/5/16 8:53 AM, Xueming Shen wrote: Hi Please help review issue: https://bugs.openjdk.java.net/browse/JDK-8166261 webre: http://cr.openjdk.java.net/~sherman/8166261/webrev The radix sanity check are missing from hasNextByte/Short/Int/Long/BigInteger(). The only method we are doing now is useRadix(). The proposed change here is to add the check into all above methods that take a radix. Arguably it's an incompatible api change, but I don't expect it really breaks anyone's code. Need go through ccc if approved. Thanks, Sherman
Re: RFR JDK-8166261: Scanner.nextInt(int) (and similar methods) throws PatternSyntaxException
Looks good to me. The test case could use IntStream.rangeClosed(Character.MIN_RADIX, Character.MAX_RADIX) for the good radixes, instead of hard coding ints. Naoto On 10/5/16 8:53 AM, Xueming Shen wrote: Hi Please help review issue: https://bugs.openjdk.java.net/browse/JDK-8166261 webre: http://cr.openjdk.java.net/~sherman/8166261/webrev The radix sanity check are missing from hasNextByte/Short/Int/Long/BigInteger(). The only method we are doing now is useRadix(). The proposed change here is to add the check into all above methods that take a radix. Arguably it's an incompatible api change, but I don't expect it really breaks anyone's code. Need go through ccc if approved. Thanks, Sherman
RFR JDK-8166261: Scanner.nextInt(int) (and similar methods) throws PatternSyntaxException
Hi Please help review issue: https://bugs.openjdk.java.net/browse/JDK-8166261 webre: http://cr.openjdk.java.net/~sherman/8166261/webrev The radix sanity check are missing from hasNextByte/Short/Int/Long/BigInteger(). The only method we are doing now is useRadix(). The proposed change here is to add the check into all above methods that take a radix. Arguably it's an incompatible api change, but I don't expect it really breaks anyone's code. Need go through ccc if approved. Thanks, Sherman
