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

[Xueming Shen]

Andrew,

Thanks for working on it.

The change looks fine, except one final nit:-)

EUC_JP_Open: #127 & #140
    it appears encoderJ0208 is no longer needed.

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

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,
>