RFR (M) JDK-8237766: Enhance signature API to include ResolvingSignatureStream
Harold Seigel
harold.seigel at oracle.com
Fri Feb 28 14:27:06 UTC 2020
Hi Lois,
This looks good! One very minor nit, could you put blank lines between
the different ResolvingSignatureStream constructors in signature.cpp,
before committing? It would make it a little more readable.
Thanks, Harold
On 2/28/2020 8:47 AM, Lois Foltan wrote:
> On 2/27/2020 7:37 PM, coleen.phillimore at oracle.com wrote:
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8237766.0/webrev/src/hotspot/share/oops/method.cpp.frames.html
>>
>>
>> 1683 // These two methods are static since a GC may move the Method
>>
>> Not in this change but can you remove this obsolete comment.
>
> Done.
>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8237766.0/webrev/src/hotspot/share/runtime/signature.cpp.frames.html
>>
>>
>> Can you make line 422 shorter? And I think the signature should be
>> the first parameter since that's the main parameter.
>
> Done. Changed both ResolvingSignatureStream constructors that have a
> signature parameter. It is now the first parameter.
>
>>
>> 452 Klass* ResolvingSignatureStream::as_loaded_klass(TRAPS) {
>>
>>
>> I think as_loaded_klass as a name doesn't really say that you're not
>> going to load the class. Can you rename klass_if_loaded()? There's a
>> klass_at_if_loaded which does the same sort of thing in the constant
>> pool.
>
> Done.
>
>>
>> This change looks good to me apart from these minor requests.
>
> Thank you for the review! Updated webrev at:
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8237766.1/webrev/
> Lois
>
>>
>> Thanks,
>> Coleen
>>
>> On 2/25/20 11:41 AM, Lois Foltan wrote:
>>> Please review the following enhancement to introduce a
>>> ResolvingSignatureStream class into the signature API support. This
>>> consolidates numerous places in the code that manually walk through
>>> the differing parts of a signature and request to resolve or find
>>> the respective underlying types. ResolvingSignatureStream now
>>> provides this capability in order to eliminate the code duplication
>>> and redundancy.
>>>
>>> open webrev at:
>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8237766.0/webrev/
>>> bug link: https://bugs.openjdk.java.net/browse/JDK-8237766
>>> contributed-by: Lois Foltan, John Rose
>>>
>>> Testing: hs-tier1-3 (all platforms), hs-tier4-6 (linux only)
>>>
>>> Thanks,
>>> Lois
>>
>
More information about the hotspot-runtime-dev
mailing list