On 9 June 2010 18:22, Xueming Shen <xueming.s...@oracle.com> wrote:
> Andrew John Hughes wrote:
>>
>> On 9 June 2010 07:55, Florian Weimer <fwei...@bfk.de> wrote:
>>
>>>
>>> * Andrew John Hughes:
>>>
>>>
>>>>
>>>> On 7 June 2010 23:04, Xueming Shen <xueming.s...@oracle.com> wrote:
>>>>
>>>>>
>>>>> Hi Andrew,
>>>>>
>>>>> 6959197: When building with JAVAC_MAX_WARNINGS=true, the build fails in
>>>>> sun/nio/cs due to the use of -Werror
>>>>>
>>>>> (1)sun/io/ByteTocharISO2022JP.java
>>>>> #129, #151
>>>>>
>>>>> if ((byte1 & (byte)0x80) != 0){
>>>>>
>>>>> if ((byte2 & (byte)0x80) != 0){
>>>>>
>>>>>
>>>>> (byte) casting is not necessary as well?
>>>>>
>>>>>
>>>>
>>>> It's necessary. 0x80 is an integer literal
>>>>
>>>> (http://java.sun.com/docs/books/jls/third_edition/html/lexical.html#3.10.1)
>>>> which requires a lossy narrowing conversion to byte
>>>>
>>>> (http://java.sun.com/docs/books/jls/third_edition/html/conversions.html#5.1.3)
>>>>
>>>
>>> Doesn't the & operator promote its arguments to int anyway? I don't
>>> think the cast to byte makes a difference here because it does not
>>> matter if 0x80 is sign-extended or zero-extended.
>>>
>>> It might be more efficient to use "byte1 < 0" and "byte2 < 0" instead.
>>>
>>>
>>
>> You're right. I think I mentally read that as an assignment, not an
>> and operation.
>>
>> If I'm reading it right now, it would seem to just be testing the sign
>> bit. byte1 < 0 would be clearer if not faster, and I'd be for
>> changing that, though that probably should be a separate patch.
>>
>> Sherman, does the new webrev look ok to push?
>>
>>
>
> (1)ByteToCharJISAutoDetect.java
> It appears we should move the maskTable1/2 initialization code out of the
> constructor block.
>
> 37 private static byte[] maskTable1 = JISAutoDetect.getByteMask1();
> 38 private static byte[] maskTable2 = JISAutoDetect.getByteMask2();
>
Done. I also made them final.
> (2)EUC_JP_Open.java
>
> 137 j0208Index1 = JIS_X_0208_Solaris_Encoder.getIndex1();
> 138 j0208Index2 = JIS_X_0208_Solaris_Encoder.getIndex2();
>
> can be moved out of the constructor as you do for the decoder. better to
> keep them consistent (as well as the code in EUC_JP_LINUX)
>
Done. I made both sets private, static and final as well, and updated
EUC_JP, EUC_JP_LINUX, PCK and SJIS in the same way.
>
> (3)sun/nio/cs/ext/HKSCS.java
>
> - this.big5Dec = big5Dec;
> + Decoder.big5Dec = big5Dec;
>
> Dave is absolutely right here. big5Dec definitely should be an instance
> field instead of static, it's my bug in the original code. It should be
>
> private DoubleByte.Decoder big5Dec;
>
> protected Decoder(Charset cs,
> DoubleByte.Decoder big5Dec,
> char[][] b2cBmp, char[][] b2cSupp)
> {
> // super(cs, 0.5f, 1.0f);
> // need to extends DoubleByte.Decoder so the
> // sun.io can use it. this implementation
> super(cs, 0.5f, 1.0f, null, null, 0, 0);
> this.big5Dec = big5Dec;
> this.b2cBmp = b2cBmp;
> this.b2cSupp = b2cSupp;
> }
>
Cool. One latent bug fixed too :-)
>
> Thanks,
> Sherman
>
>
>
>
>
New webrev: http://cr.openjdk.java.net/~andrew/warnings/webrev.05/
Ok to push?
Thanks,
--
Andrew :-)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net
PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA 7927 142C 2591 94EF D9D8
