RFR[9]:Fix java/lang/invoke/MethodHandleImpl's use of Unsafe.defineAnonymousClass()
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Thu May 12 12:42:05 UTC 2016
Overall, looks really good! I like how it shapes out.
A couple of nitpicks:
(1) Please, use more meaningful names: INJECTED_INVOKER_TEMPLATE vs
T_BYTES, invokerTemplateGenerator vs tBytesGenerator, InjectedInvoker vs T.
(2) There's no need in a constructor. The invoker is never instantiated.
(3) I don't see any point in T.init(). It is called from
BindCaller.makeInjectedInvoker [1], but: (1) it's initialized later
anyway [2]; and (2) Unsafe.ensureClassInitialized can be used.
Frankly speaking, I'd prefer [2] to be converted into an assert. Also, I
don't see any point in forcing the initialization of the invoker class
during construction. I think it can go away as well.
Best regards,
Vladimir Ivanov
[1]
http://hg.openjdk.java.net/jdk9/dev/jdk/file/698b526d7c3b/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java#l1123
[2]
http://hg.openjdk.java.net/jdk9/dev/jdk/file/698b526d7c3b/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java#l1137
On 5/12/16 3:02 PM, shilpi.rastogi at oracle.com wrote:
> Thank You Paul for comments.
> Please review updated webrev
> http://cr.openjdk.java.net/~srastogi/8149574/webrev.08/
>
> Regards,
> Shilpi
>
> On 5/12/2016 3:41 PM, Paul Sandoz wrote:
>>> On 11 May 2016, at 18:31, shilpi.rastogi at oracle.com wrote:
>>>
>>> Hi All,
>>>
>>> Please review the updated webrev-
>>> http://cr.openjdk.java.net/~srastogi/8149574/webrev.07/
>>>
>>
>> 1219 FieldVisitor fv;
>> 1220 MethodVisitor mv;
>> 1221 AnnotationVisitor av0;
>>
>> Field “fv is not used. Since “av0” is only used once, might as well
>> declare it at line #1246.
>>
>> Can you break up the long lines at #1242 & #1252 ?
>>
>>
>> My inclination is to turn the anon static block into a method
>> returning byte[] and then do:
>>
>> /*
>> <JAVA DOC explaining the class that is generated rather than just
>> floating as is the current case>
>> */
>> private static final byte[] T_BYTES = generateT();
>> private static byte[] generateT() {
>> …
>> }
>>
>>
>> One concern, more so because of my ignorance, is why can the default
>> package be used. Does anyone know?
>>
>> Paul.
>>
>>> Changed the anonymous class package with no package name.
>>>
>>> Regards,
>>> Shilpi
>>>
>
More information about the core-libs-dev
mailing list