Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-04 Thread Ivan Gerasimov
On 9/4/18 9:40 AM, joe darcy wrote: Hello, Please also create a quick CSR to cover the behavioral change. Okay, here it is: https://bugs.openjdk.java.net/browse/JDK-8210377 Would you please help review it? Thanks in advance! Ivan Thanks, -Joe On 8/31/2018 5:17 PM, Ivan Gerasimov

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-04 Thread Ivan Gerasimov
Thanks everyone for reviewing! As suggested, I added a test to cover CharsetEncoder constructor and a sanity check that the constructor doesn't throw given valid arguments. http://cr.openjdk.java.net/~igerasim/8210285/01/webrev/ With kind regards, Ivan On 9/3/18 8:31 AM, Martin Buchholz

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-04 Thread joe darcy
Hello, Please also create a quick CSR to cover the behavioral change. Thanks, -Joe On 8/31/2018 5:17 PM, Ivan Gerasimov wrote: Hello! The javadoc for CharsetDecoder [1] states that an exception is thrown when a non-positive number is passed in as an argument. However we only reject

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-03 Thread Martin Buchholz
Looks good to me. I would add a call to new MyDecoder(ascii, 0.5f, 1.5f) to make sure all calls to the constructor don't throw (because e.g. for ASCII we know the correct values are 1.0f). (In any case it feels like an API design mistake - the Charset itself should be the source of truth about

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-03 Thread Alan Bateman
On 03/09/2018 05:54, Ivan Gerasimov wrote: Thanks Sherman and Stuart for the review! On 9/2/18 2:45 PM, Stuart Marks wrote: Yes, the fix itself looks fine. Quite subtle, good catch. But should this have a regression test? I can imagine somebody coming along later and "simplifying" (!(... >

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-02 Thread Ivan Gerasimov
Thanks Sherman and Stuart for the review! On 9/2/18 2:45 PM, Stuart Marks wrote: Yes, the fix itself looks fine. Quite subtle, good catch. But should this have a regression test? I can imagine somebody coming along later and "simplifying" (!(... > ...)) to (... <= ...) which would

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-02 Thread Stuart Marks
Yes, the fix itself looks fine. Quite subtle, good catch. But should this have a regression test? I can imagine somebody coming along later and "simplifying" (!(... > ...)) to (... <= ...) which would reintroduce the bug. s'marks On 9/2/18 11:14 AM, Xueming Shen wrote: +1 On 8/31/18, 5:17

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-09-02 Thread Xueming Shen
+1 On 8/31/18, 5:17 PM, Ivan Gerasimov wrote: Hello! The javadoc for CharsetDecoder [1] states that an exception is thrown when a non-positive number is passed in as an argument. However we only reject negative or zero numbers, but not NaN. And likewise for CharsetEncoder. Would you

RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

2018-08-31 Thread Ivan Gerasimov
Hello! The javadoc for CharsetDecoder [1] states that an exception is thrown when a non-positive number is passed in as an argument. However we only reject negative or zero numbers, but not NaN. And likewise for CharsetEncoder. Would you please help review a trivial fix? BUGURL: