On 10 June 2010 23:11, Xueming Shen <xueming.s...@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.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, >> > > 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
