Codereview request for 7096080: UTF8 update and new CESU-8 charset
Ulf Zibis
Ulf.Zibis at gmx.de
Mon Nov 7 21:41:59 UTC 2011
I have thought that and it was easy to see, as it glowed in blue in Sdiffs.
But I anyway wondered about the combination of protected with final, or is there some miracle I
don't know?
And the diff line is still glowing ...
You can drop final as for class Decoder ;-)
-Ulf
Am 07.11.2011 22:28, schrieb Xueming Shen:
> Hi Ulf,
>
> Good catch. It was the leftover of my first drift, in which CESU.de/encoder is the
> subclass of the UTF8 de/encoder.
>
> webrev has been updated accordingly.
>
> -Sherman
>
> On 11/07/2011 12:57 PM, Ulf Zibis wrote:
>> Hi Sherman,
>>
>> 2 nits again:
>> protected static final class Encoder extends CharsetEncoder
>> - If final, protected doesn't make sense, as it can't be subclassed. (ideally this should be a
>> compiler error)
>> - Where is it used? Otherwise it could be private.
>>
>> -Ulf
>>
>>
>> Am 07.11.2011 21:30, schrieb Xueming Shen:
>>> Thanks Alan,
>>>
>>> The webrev has been updated accordingly to address your comments (the
>>> commented out isMalformed2 has been removed, copyright updated, bugid
>>> added...)
>>>
>>> And thanks for Ulf for the detailed review.
>>>
>>> -Sherman
>>>
>>> On 10/28/2011 09:14 AM, Alan Bateman wrote:
>>>> On 28/09/2011 20:18, Xueming Shen wrote:
>>>>> Hi,
>>>>>
>>>>> [I combined the proposed charge for #7082884, in which no one appears to be
>>>>> interested:-) into this one]
>>>>> :
>>>>>
>>>>> http://cr.openjdk.java.net/~sherman/7096080/webrev/
>>>>> <http://cr.openjdk.java.net/%7Esherman/7096080/webrev/>
>>>> I don't know if you are still looking for a reviewer for this (seems like Ulf has gone through
>>>> this in detail, thanks Ulf).
>>>>
>>>> Overall it looks fine to me. Minor comment is that in UTF_8.java then maybe isMalformed2 should
>>>> be removed completely, maybe move some of the comment in the decode methods. Another minor nits
>>>> is that the date on CESU_8.java is 2000-2010 where I assume it should be 2011. In Errors.java
>>>> then it might be better to just remove L196 as it might confuse future maintainers. I would
>>>> also suggest adding the bugID to the list of bugs in the tests too as someone these references
>>>> are useful.
>>>>
>>>> -Alan.
>>>
>>>
>
>
More information about the core-libs-dev
mailing list