I have updated the patch to reflect Alan's remarks. The webrev is at the same location - github takes care of versioning ...
-JB- On 10/04/2012 05:56 PM, Jaroslav Bachorik wrote: > On Thu 04 Oct 2012 05:42:15 PM CEST, Alan Bateman wrote: >> On 04/10/2012 16:28, Jaroslav Bachorik wrote: >>> : >>> This is a follow-up. I've prepared the patch and put it on github - >>> https://github.com/jbachorik/openjdk-patches/tree/master/webrevs/7195779 >>> >>> I wonder who else should be included in the review process since I am >>> changing the IIOP generator code. Also, I didn't find any tests in the >>> corba repository. Which test suite is appropriate to run after changing >>> the corba related code? >>> >>> -JB- >>> >> I don't mind being reviewer and sponsor for this. Also cc'ing Sean as >> he is one of the maintainers of the corba code. I don't think the > > Thanks. > >> corba tests are in OpenJDK, at least I don't think Oracle has >> contributed its tests for this area. >> >> I think your change looks okay and I assume you've at least run the >> JMX tests that use RMI-IIOP to verify that the intermittent NPE is >> gone and those tests now pass reliably. > > Yes, I ran those. Especially on the test machine yielding the biggest > ration of failed tests previously. Now they all pass. > >> >> Minor comment but if I were doing this myself then I probably would >> have added this instead: >> p.pln(getName(theType) + " target = this.target;"); >> >> You'll see lots of examples of this in the core libs and j.u.c. > > No problem. If it is a convention I will stick to it (I've used a > different variable name to prevent confusion about what is a field and > what is a local variable). > >> >> Also as target is now volatile then I'm not sure why you synchronized >> around target=null, perhaps there is other code generated in the tie >> class that I don't see? > > The synchronization should not be there. It just escaped my purging > when I've exchanged the synchronized access for volatile. > > -JB- > >> >> Otherwise it's great to get issue finally resolved. >> >> -Alan. >> >> > >
