RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter
Roger Riggs
Roger.Riggs at Oracle.com
Mon Dec 11 19:18:33 UTC 2017
Hi Joe,
The new tests using testng look good, a few comments:
- BAOS/EncodingTest.java
70: The message gets printed if the condition is not true; so
"results do not might" might read better
(also, there is Assert.assertEquals(str1, str2, msg) - that
compares objects with .equals())
A message in nice, though in many cases the testng generated
exception/message is sufficient
especially for equals.
- PrintStream/EncodingTest.java
92: out should not be null; its probably a test bug; the test
should be removed
- PrintWriter/EncodingTest.java
93: out should not be null; its probably a test bug; the test
should be removed
Thanks, Roger
On 12/8/2017 7:29 PM, Joe Wang wrote:
> Thanks Roger, Alan for the comments!
>
> I've updated the webrev accordingly. Here's the current webrev:
> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>
> -Joe
>
> On 12/4/17, 7:52 PM, Joe Wang wrote:
>>
>>
>> On 12/3/17, 11:34 AM, Alan Bateman wrote:
>>>
>>>
>>> On 01/12/2017 20:35, Joe Wang wrote:
>>>> :
>>>>
>>>>>
>>>>> I think URLDecoder.decode methods should have @throws
>>>>> IllegalArgumentException. I realize it's implementation specific
>>>>> as to whether IAE is thrown with bad input but given that the RI
>>>>> does throw IAE then consumers of the API should be prepared for
>>>>> it. The @implNote can stay and probably should be copied into the
>>>>> legacy decode method too.
>>>>
>>>> I added @throws IAE. On a 2nd thought, would that give no
>>>> flexibility to alternative impls as the general (class) spec had
>>>> given? With this addition, it becomes a requirement.
>>> If the @throws description starts with "if the implementation ..."
>>> then it should be clear that it is optional.
>>
>> Thanks Alan. I made the change accordingly.
>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>
>> -Joe
>>
>>>
>>> -Alan
More information about the core-libs-dev
mailing list