----- Original Message ----- > Hi Andrew, > > thanks for the review. I am pushing the change. > > Sorry for the mess. >
It's ok. I'm still baffled that javac misses it. I was able to build with OpenJDK6, just not with IcedTea's bootstrapping setup, which uses ecj. Thanks for the quick fix. > Thanks, > Andrew > > On 3/7/2013 7:12 PM, Andrew Hughes wrote: > > ----- Original Message ----- > >> Hello, > >> > >> please review a fix for the build failure: > >> > >> http://cr.openjdk.java.net/~bae/8009641/webrev.00/ > >> > > Looks good to me and better than what I had as the synchronized > > blocks were still outside the try > > (so the variables could end up being null). > > > > Ok to commit. > > > >> Thanks, > >> Andrew > >> > >> On 3/7/2013 5:55 PM, Seán Coffey wrote: > >>> Even better if you want to push the change ? I haven't heard from > >>> Andrew B. yet. > >>> > >>> I've logged 8009641 to track this. You could use that ID if you > >>> want. > >>> > >>> 8009641: OpenJDK 6 build broken via 8007675 fix > >>> > >>> regards, > >>> Sean. > >>> > >>> On 07/03/2013 13:51, Andrew Hughes wrote: > >>>> ----- Original Message ----- > >>>>> Yes - you're right. That does look like an issue. Andrew > >>>>> Brygin > >>>>> ran > >>>>> pre > >>>>> integration tests before pushing the changes internally and > >>>>> they > >>>>> were > >>>>> successful. However - I've traced back over the sources and > >>>>> what > >>>>> was > >>>>> run > >>>>> in his test build and what he pushed to internal repo differs. > >>>>> ( > >>>>> in 2 > >>>>> areas) - No doubt, it's the curse of the last minute change. > >>>>> > >>>>> Andrew Brygin, can you log a bug and get this corrected asap ? > >>>>> > >>>>> Thanks for the notice Andrew! > >>>>> > >>>> I have a patch I can post if that would help. With that, this > >>>> builds. > >>>> I just need to clean up my workspace and post a webrev. > >>>> > >>>>> Thanks, > >>>>> Sean. > >>>>> > >>>>> > >>>>>> public short[] colorConvert(short[] src, short[] dst) { > >>>>>> > >>>>>> if (dst == null) { > >>>>>> dst = new short > >>>>>> [(src.length/getNumInComponents())*getNumOutComponents()]; > >>>>>> } > >>>>>> > >>>>>> try { > >>>>>> LCMSImageLayout srcIL = new LCMSImageLayout( > >>>>>> src, src.length/getNumInComponents(), > >>>>>> LCMSImageLayout.CHANNELS_SH(getNumInComponents()) | > >>>>>> LCMSImageLayout.BYTES_SH(2), > >>>>>> getNumInComponents()*2); > >>>>>> > >>>>>> LCMSImageLayout dstIL = new LCMSImageLayout( > >>>>>> dst, dst.length/getNumOutComponents(), > >>>>>> LCMSImageLayout.CHANNELS_SH(getNumOutComponents()) > >>>>>> | > >>>>>> LCMSImageLayout.BYTES_SH(2), > >>>>>> getNumOutComponents()*2); > >>>>>> > >>>>>> synchronized(this) { > >>>>>> LCMS.colorConvert(this, srcIL, dstIL); > >>>>>> } > >>>>>> } catch (ImageLayoutException e) { > >>>>>> throw new CMMException("Unable to convert > >>>>>> data"); > >>>>>> } > >>>>>> > >>>>>> return dst; > >>>>>> } > >>>>> On 07/03/2013 12:39, Andrew Hughes wrote: > >>>>>> ----- Original Message ----- > >>>>>>> ----- Original Message ----- > >>>>>>>> I'm only the proxy here but I created the webrev from the > >>>>>>>> changesets > >>>>>>>> that I pushed. I don't see any difference. > >>>>>>>> > >>>>>>>> t4 $diff LCMSTransform.java.webrev > >>>>>>>> jdk/src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java > >>>>>>>> t4 $ > >>>>>>>> > >>>>>>> Indeed. I'm still seeing the same failure using the > >>>>>>> changesets > >>>>>>> pushed > >>>>>>> to OpenJDK6, yet the repository itself builds. Very odd. > >>>>>>> > >>>>>> The copy of OpenJDK6 I was looking at wasn't up-to-date, hence > >>>>>> the > >>>>>> diff. > >>>>>> The patched version and upstream repo are identical when > >>>>>> compared > >>>>>> correctly. > >>>>>> > >>>>>> What's baffling is how this code compiles with the javac > >>>>>> compiler > >>>>>> as it looks > >>>>>> clearly wrong to me, which is why ecj falls over on it. > >>>>>> > >>>>>> try { > >>>>>> LCMSImageLayout srcIL = new LCMSImageLayout( > >>>>>> src, src.length/getNumInComponents(), > >>>>>> LCMSImageLayout.CHANNELS_SH(getNumInComponents()) > >>>>>> | > >>>>>> LCMSImageLayout.BYTES_SH(2), > >>>>>> getNumInComponents()*2); > >>>>>> > >>>>>> LCMSImageLayout dstIL = new LCMSImageLayout( > >>>>>> dst, dst.length/getNumOutComponents(), > >>>>>> LCMSImageLayout.CHANNELS_SH(getNumOutComponents()) > >>>>>> | > >>>>>> LCMSImageLayout.BYTES_SH(2), > >>>>>> getNumOutComponents()*2); > >>>>>> } catch (ImageLayoutException e) { > >>>>>> throw new CMMException("Unable to convert > >>>>>> data"); > >>>>>> } > >>>>>> > >>>>>> synchronized(this) { > >>>>>> LCMS.colorConvert(this, srcIL, dstIL); > >>>>>> } > >>>>>> > >>>>>> srcIL and dstIL are declared inside the try block so aren't > >>>>>> visible > >>>>>> outside it. > >>>>>> > >>>>>> In the other colorConvert methods, these are declared at the > >>>>>> top > >>>>>> of > >>>>>> the method. > >>>>>> > >>>>>>>> regards, > >>>>>>>> Sean. > >>>>>>>> > >>>>>>>> On 07/03/2013 10:59, Andrew Hughes wrote: > >>>>>>>>> ----- Original Message ----- > >>>>>>>>>> February CPU pushes completed as reviewed in last round of > >>>>>>>>>> webrevs. > >>>>>>>>>> > >>>>>>>>>> I'd like to push 2 extra fixes now for issues addressed in > >>>>>>>>>> yesterday's > >>>>>>>>>> JDK releases. > >>>>>>>>>> > >>>>>>>>>> webrev : > >>>>>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.6open.mar5/ > >>>>>>>>>> > >>>>>>>>>> Good to push ? > >>>>>>>>>> > >>>>>>>>>> regards, > >>>>>>>>>> Sean. > >>>>>>>>>> > >>>>>>>>>> On 05/03/2013 18:44, Omair Majid wrote: > >>>>>>>>>>> On 03/05/2013 10:52 AM, Edvard Wendelin wrote: > >>>>>>>>>>>> Hi, > >>>>>>>>>>>> > >>>>>>>>>>>> The change in JAXP can be viewed here: > >>>>>>>>>>>> http://cr.openjdk.java.net/~joehw/jdk8/8001235/webrev/ > >>>>>>>>>>>> While > >>>>>>>>>>>> it's > >>>>>>>>>>>> generated against JDK 8, the change in 6 is identical. > >>>>>>>>>>> The JAXP changes look identical to what was pushed to > >>>>>>>>>>> jdk7u. > >>>>>>>>>>> Looks > >>>>>>>>>>> all > >>>>>>>>>>> good to me! > >>>>>>>>>>> > >>>>>>>>>>>> I plan to push the changes today. > >>>>>>>>>>> Thank you. It will be great to have all the security > >>>>>>>>>>> fixes > >>>>>>>>>>> in! > >>>>>>>>>>> > >>>>>>>>>>> Cheers, > >>>>>>>>>>> Omair > >>>>>>>>>>> > >>>>>>>>> I notice that what was finally committed differs from this > >>>>>>>>> webrev > >>>>>>>>> (in a positive > >>>>>>>>> way). The version posted from the webrev failed to > >>>>>>>>> compile: > >>>>>>>>> > >>>>>>>>> 1. ERROR in > >>>>>>>>> ../../../src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java > >>>>>>>>> (at line 580) > >>>>>>>>> LCMS.colorConvert(this, srcIL, dstIL); > >>>>>>>>> ^^^^^ > >>>>>>>>> srcIL cannot be resolved to a variable > >>>>>>>>> ---------- > >>>>>>>>> 2. ERROR in > >>>>>>>>> ../../../src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java > >>>>>>>>> (at line 580) > >>>>>>>>> LCMS.colorConvert(this, srcIL, dstIL); > >>>>>>>>> ^^^^^ > >>>>>>>>> dstIL cannot be resolved to a variable > >>>>>>>>> ---------- > >>>>>>>>> 3. ERROR in > >>>>>>>>> ../../../src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java > >>>>>>>>> (at line 606) > >>>>>>>>> LCMS.colorConvert(this, srcIL, dstIL); > >>>>>>>>> ^^^^^ > >>>>>>>>> srcIL cannot be resolved to a variable > >>>>>>>>> ---------- > >>>>>>>>> 4. ERROR in > >>>>>>>>> ../../../src/share/classes/sun/java2d/cmm/lcms/LCMSTransform.java > >>>>>>>>> (at line 606) > >>>>>>>>> LCMS.colorConvert(this, srcIL, dstIL); > >>>>>>>>> ^^^^^ > >>>>>>>>> dstIL cannot be resolved to a variable > >>>>>>>>> > >>>>>>>>> but this seems to have been fixed in the committed version, > >>>>>>>>> presumably due to: > >>>>>>>>> > >>>>>>>>> - try { > >>>>>>>>> - LCMSImageLayout srcIL = new LCMSImageLayout( > >>>>>>>>> - src, src.length/getNumInComponents(), > >>>>>>>>> - > >>>>>>>>> LCMSImageLayout.CHANNELS_SH(getNumInComponents()) > >>>>>>>>> | > >>>>>>>>> - LCMSImageLayout.BYTES_SH(1), > >>>>>>>>> getNumInComponents()); > >>>>>>>>> - > >>>>>>>>> - LCMSImageLayout dstIL = new LCMSImageLayout( > >>>>>>>>> - dst, dst.length/getNumOutComponents(), > >>>>>>>>> - > >>>>>>>>> LCMSImageLayout.CHANNELS_SH(getNumOutComponents()) > >>>>>>>>> | > >>>>>>>>> - LCMSImageLayout.BYTES_SH(1), > >>>>>>>>> getNumOutComponents()); > >>>>>>>>> - } catch (ImageLayoutException e) { > >>>>>>>>> - throw new CMMException("Unable to convert > >>>>>>>>> data"); > >>>>>>>>> - } > >>>>>>>>> + LCMSImageLayout srcIL = new LCMSImageLayout( > >>>>>>>>> + src, src.length/getNumInComponents(), > >>>>>>>>> + LCMSImageLayout.CHANNELS_SH(getNumInComponents()) | > >>>>>>>>> + LCMSImageLayout.BYTES_SH(1), > >>>>>>>>> getNumInComponents()); > >>>>>>>>> + > >>>>>>>>> + LCMSImageLayout dstIL = new LCMSImageLayout( > >>>>>>>>> + dst, dst.length/getNumOutComponents(), > >>>>>>>>> + LCMSImageLayout.CHANNELS_SH(getNumOutComponents()) > >>>>>>>>> | > >>>>>>>>> + LCMSImageLayout.BYTES_SH(1), > >>>>>>>>> getNumOutComponents()); > >>>>>>>>> > >>>>>>>>> so srcIL and dstIL are now declared at a wider scope. I'll > >>>>>>>>> use > >>>>>>>>> the > >>>>>>>>> committed versions > >>>>>>>>> in IcedTea6 HEAD. > >>>>>>>>> > >>>>>>>>> Cheers, > >>>>>>> -- > >>>>>>> Andrew :) > >>>>>>> > >>>>>>> Free Java Software Engineer > >>>>>>> Red Hat, Inc. (http://www.redhat.com) > >>>>>>> > >>>>>>> PGP Key: 248BDC07 (https://keys.indymedia.org/) > >>>>>>> Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B > >>>>>>> DC07 > >>>>>>> > >>>>>>> > >> > > -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
