RFR 8009382: Add JVM_Get{Field|Method}TypeAnnotations
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Mar 26 13:05:43 UTC 2013
Thumbs up on the re-review.
Dan
On 3/26/13 4:32 AM, Joel Borggrén-Franck wrote:
> Hi,
>
> Also including build-dev since this touches some makefiles.
>
> Thanks Dan! see inline, also new webrev:
>
> http://cr.openjdk.java.net/~jfranck/8009382/hotspot/webrev.02/
>
> On 03/25/2013 04:17 PM, Daniel D. Daugherty wrote:
>> On 3/22/13 8:16 AM, Joel Borggrén-Franck wrote:
>
>> make/bsd/makefiles/mapfile-vers-debug
>> make/linux/makefiles/mapfile-vers-debug
>> make/linux/makefiles/mapfile-vers-product
>> make/solaris/makefiles/mapfile-vers
>> It looks like the other entries are in alpha order so these
>> two new entries should also be added in alpha order.
>>
>
> Fixed. I also noticed I forgot to update one of the bsd makefiles.
>
>> src/share/vm/prims/jvm.cpp
>> line 1510: return NULL;
>> nit: indent should be two spaces
>>
>> line 1529: assert(false, "cannot find method");
>> line 1530: return NULL; // robustness
>> Normally "robustness" comments flag logic after an assert()
>> to indicate what we do in product mode to deal with the "bad"
>> situation without crashing. In this case, we don't try to use
>> 'm' after discovering that it is NULL so this could be simpler:
>>
>> Method* m =
>> InstanceKlass::cast(k)->method_with_idnum(slot);
>> assert(m != NULL, "cannot find method");
>> return m; // caller has to deal with NULL in product mode
>>
>> Yes, I realize you only touched the comment here, but it
>> served to point out the messiness of the existing code.
>>
>
> Good suggestion. Fixed.
>
>> line 1551: return NULL;
>> line 1565: return NULL;
>> line 1579: return NULL;
>> nit: indent should be two spaces
>>
>> line 1632: return NULL;
>> line 1613: return NULL;
>> nit: indent should be two spaces
>
> Fixed all indent nits.
>
>>>
>>> A prototype of the jdk changes can be found here:
>>> http://cr.openjdk.java.net/~jfranck/8009382/jdk/webrev.00/
>>
>> You should use a separate bug ID for the jdk repo changes. This will
>> prevent confusion between when HSX with these changes is integrated
>> into JDK8 and when the jdk repo changes are integrated into JDK8.
>>
>
> The JDK changes have a separate bug, sorry if this was unclear. The
> jdk changes shown are just a proof of concept, intended to highlight
> that my HotSpot changes have actually been run. I will send out a
> separate review request in the future for the JDK changes.
>
> cheers
> /Joel
More information about the build-dev
mailing list