PING: Re: Fix build failure with JAVAC_MAX_WARNINGS=true in sun/nio/cs

[Andrew John Hughes]

On 9 June 2010 18:22, Xueming Shen <xueming.shen at oracle.com> wrote:
> Andrew John Hughes wrote:
>>
>> On 9 June 2010 07:55, Florian Weimer <fweimer at bfk.de> wrote:
>>
>>>
>>> * Andrew John Hughes:
>>>
>>>
>>>>
>>>> On 7 June 2010 23:04, Xueming Shen <xueming.shen at 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