RFR 8031820: NPG: Fix remaining references to metadata as oops in comments

Coleen Phillimore coleen.phillimore at oracle.com
Fri Mar 21 15:57:23 UTC 2014


Thank you Stefan for reading the new comments!

On 3/21/14 5:02 AM, Stefan Karlsson wrote:
>
> On 2014-03-20 20:39, Coleen Phillimore wrote:
>> Also fixed:
>> 8012125: Comments for ConstantPoolCache should reflect the addition 
>> of resolved_references in ConstantPool
>> Summary: Updated comments in metadata header files, and renamed 
>> this_oop variables to this_cp or this_k when referring to constant 
>> pool or classes.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8031820/
>
> Looks good. Thanks for cleaning this up!
>
>
> Small comments:
>
> http://cr.openjdk.java.net/~coleenp/8031820/src/share/vm/oops/constMethod.hpp.frames.html 
>
>
>  42 // critical at all.  Accessing the checked exceptions table is 
> used by reflection,
>  43 // so we put that last to make access to it fast.
>
> It doesn't look like we put the checked exceptions table at the end of 
> the constMethod.

You're right.   This was an old comment moved.  I reworked it a bit.   See:

http://cr.openjdk.java.net/~coleenp/8031820_2


>
>
> http://cr.openjdk.java.net/~coleenp/8031820/src/share/vm/oops/constantPool.hpp.udiff.html 
>
>
>    friend class VMStructs;
> -  friend class BytecodeInterpreter;  // Directly extracts an oop in 
> the pool for fast instanceof/checkcast
> +  friend class BytecodeInterpreter;  // Directly extracts an klass in 
> the pool for fast instanceof/checkcast
> +
>    friend class Universe;             // For null constructor
>
> Do we need/want this extra blank line?
>

I took it out.
>
> http://cr.openjdk.java.net/~coleenp/8031820/src/share/vm/oops/cpCache.hpp.udiff.html 
>
>
> I haven't verified that this comment is correct.
>
>
> Not related to this patch, but is there a reason why all these 
> functions are static and take a constantPoolHandle as the first 
> parameter, instead of being instance member functions? My guess is 
> that it's just to make sure MetadataOnStackMark finds this_cp.
>
> +  static Method*          method_at_if_loaded (constantPoolHandle 
> this_cp, int which);
> +  static bool       has_appendix_at_if_loaded (constantPoolHandle 
> this_cp, int which);
> +  static oop            appendix_at_if_loaded (constantPoolHandle 
> this_cp, int which);
> +  static bool    has_method_type_at_if_loaded (constantPoolHandle 
> this_cp, int which);
> +  static oop         method_type_at_if_loaded (constantPoolHandle 
> this_cp, int which);
> +  static Klass*            klass_at_if_loaded (constantPoolHandle 
> this_cp, int which);
> +  static Klass*        klass_ref_at_if_loaded (constantPoolHandle 
> this_cp, int which);
>

It's historical baggage when they were oops but probably 
MetadataOnStackMark needs these to be handles, so I've left them in.  I 
think slowly we can turn them into instance member functions but not yet.

Thanks!

Coleen
>
> thanks,
> StefanK
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8031820
>>
>> Tested with jck, all jtreg hotspot tests and nsk vm.quick.testlist.
>>
>> Thanks,
>> Coleen
>



More information about the hotspot-runtime-dev mailing list