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 wrote:
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 charsPerByte)
On Mon, Sep 3, 2018 at 3:57 AM, Alan Bateman <alan.bate...@oracle.com
<mailto:alan.bate...@oracle.com>> wrote:
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" (!(... >
...)) to (... <= ...) which would reintroduce the bug.
Yes, there is a regression test added:
http://cr.openjdk.java.net/~igerasim/8210285/00/webrev/test/jdk/java/nio/charset/CharsetDecoder/NaNinCtor.java.html
<http://cr.openjdk.java.net/%7Eigerasim/8210285/00/webrev/test/jdk/java/nio/charset/CharsetDecoder/NaNinCtor.java.html>
The update to Charset-X-Coder.java.template looks okay. The
changes also CharsetEncoder so we'll need test coverage for that too.
-Alan
--
With kind regards,
Ivan Gerasimov
