Why not add the required code when/if it's actually needed? It's not like the pattern is tricky :) your call of course, just looks odd as-is.
Sent from my phone On Sep 26, 2013 8:15 PM, "Christian Thalinger" < christian.thalin...@oracle.com> wrote: > > On Sep 26, 2013, at 5:11 PM, Vitaly Davidovich <vita...@gmail.com> wrote: > > Chris, > > Since you touched getName(), maybe get rid of that superfluous null check > after expandFromVM? The code can just fall through. > > > Was thinking about removing it and already had it but backed of because I > wanted to keep the pattern if someone adds code to that method in the > future. The compiler will optimize it anyway. > > Vitaly > > Sent from my phone > On Sep 26, 2013 5:47 PM, "Christian Thalinger" < > christian.thalin...@oracle.com> wrote: > >> >> On Sep 26, 2013, at 2:28 PM, John Rose <john.r.r...@oracle.com> wrote: >> >> > On Sep 26, 2013, at 1:13 PM, Christian Thalinger < >> christian.thalin...@oracle.com> wrote: >> > >> >> On Sep 26, 2013, at 11:50 AM, Christian Thalinger < >> christian.thalin...@oracle.com> wrote: >> >> >> >>> >> >>> On Sep 26, 2013, at 1:22 AM, Peter Levart <peter.lev...@gmail.com> >> wrote: >> >>> >> >>>> On 09/26/2013 01:27 AM, Christian Thalinger wrote: >> >>>>> http://cr.openjdk.java.net/~twisti/8019192/webrev/ >> >>>>> >> >>>>> 8019192: StringIndexOutOfBoundsException: in Class.getSimpleName() >> >>>>> Reviewed-by: >> >>>>> >> >>>>> This is a race in MemberName's name and type getters. >> >>>>> >> >>>>> MemberName's type field is of type Object so it can hold different >> objects when it gets filled in from the VM. These types include String and >> Object[]. On the first invocation the current type if it's not MethodType >> gets converted to a MethodType. >> >>>>> >> >>>>> There is a tiny window where some instanceof check have already >> been done on one thread and then another thread stores a MethodType. The >> following checkcast then fails. >> >>>>> >> >>>>> The fix is to make name and type volatile and do the conversion in >> a synchronized block. This is okay because it's only done once. >> >>>>> >> >>>>> src/share/classes/java/lang/invoke/MemberName.java >> >>>>> >> >>>> >> >>>> Hi Christian, >> >>>> >> >>>> Wouldn't it be cleaner that instead of just casting and catching >> ClassCastException, the volatile field 'type' was 1st copied to a local >> variable and then an instanceof check + casting and returning performed on >> the local variable. This would avoid throwing ClassCastException even if it >> is performed only once per MemberName… >> >>> >> >>> Not sure it would be cleaner; depends on the definition of "cleaner". >> I had similar code as you describe before but I changed it to catch the >> exception. If people have a strong opinion here I can change it back. >> >> >> >> Here are the two different versions: >> >> >> >> http://cr.openjdk.java.net/~twisti/8019192/webrev.00/ >> >> http://cr.openjdk.java.net/~twisti/8019192/webrev.01/ >> > >> > I think the second version is better. The first uses exception >> processing for normal (though rare) control flow, which is questionable >> style. >> > >> > As we discussed offline, the fields do *not* need to be volatile, since >> the code in the synch. block will see an updated state without volatiles, >> and races outside the synch block are benign, leading to the same end-state. >> > >> > Hoisting the field into a local is the best practice here.(***) That's >> the hand we are dealt with the Java memory model. >> > >> > Reviewed, with those adjustments. >> >> …and there is the according webrev: >> >> http://cr.openjdk.java.net/~twisti/8019192/webrev.02/ >> >> > >> > — John >> > >> > (***) P.S. This tickles one of my pet peeves. A hoisted field value >> should have the same name (either approximately or exactly) as the field, >> as long as the difference is merely the sampling of a value which is not >> expected to change. (As opposed to a clever saving of a field that is >> destined to be updated, etc.) >> > >> > IDEs and purists may disagree with this, but it always sets my teeth on >> edge to see name changes that obscure consistency of values for the sake of >> some less interesting consistency (scope rules or rare state changes). >> >> The problem I have with this is that some people might think the local >> variable is the instance field. IDEs help here because they usually >> highlight local variables and instance fields in different colors. How >> about this: >> >> http://cr.openjdk.java.net/~twisti/8019192/webrev.03/ >> >> > >> > Is there at least some standard naming pattern for these things? >> "typeSnapshot" doesn't do it for me. "typeVal" is quieter; I prefer >> quieter. Elsewhere in the same code I have defiantly used plain "type" >> with a shout-out to hostile IDEs, as with MethodHandle.bindTo or >> MethodHandle.asCollector. Looking for a compromise... >> > >> > (I also firmly believe that constructor parameter names are best named >> with the corresponding field names, even though quibblers of various >> species, IDE/human, note that there are distinctions to be made between >> them. It is for the sake of this opinion of mine that blank finals can be >> initialized with the form 'this.foo = foo', which some dislike.) >> >> I concur. This is what I'm doing. >> >> > >> > But it's fine; push it! >> >> >
