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

[Andrew John Hughes]

On 10 June 2010 23:11, Xueming Shen <xueming.shen at oracle.com> wrote:
> Andrew,
>
> Thanks for working on it.
>

Thanks for such a through review process.

> The change looks fine, except one final nit:-)
>
> EUC_JP_Open: #127 & #140
>   it appears encoderJ0208 is no longer needed.
>

Fixed.

> Please run those tests at sun/nio/cs before push, I don't really trust my
> own eyes
> these days:-)
>

Everything passed that passed before (sun/nio/cs/TestStringCoding
failed before and after with a SecurityException) so I pushed the fix:

http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c849dc20dc85

> Sherman
>
>
> Andrew John Hughes wrote:
>>
>> 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,
>>
>
>


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