Request for review 8000725: NPG: method_holder() and pool_holder() and _pool_holder field should be InstanceKlass

[harold seigel]

Hi David,

We will have a webrev for review later this week that will remove the 
Klass::cast() function and all its uses (see bug 8001471).  We thought 
it would be easier to just remove the unnecessary InstanceKlass::cast() 
calls in this webrev (along with a few Klass::cast() calls).

Thanks for reviewing it!

Harold

On 10/27/2012 4:02 AM, David Holmes wrote:
> Hi Harold,
>
> On 27/10/2012 4:11 AM, harold seigel wrote:
>> Please review the following changes.
>>
>> Summary: Change the function return type of method_holder(),
>> field_holder(), and pool_holder() to InstanceKlass. Also, change the
>> type of field _pool_holder to InstanceKlass. These changes enabled the
>> removal of lots of InstanceKlass::cast() calls from the code.
>
> There have also been a lot of removals of Klass::cast even though the 
> previous return types were Klass*. So looking at Klass::cast I see
>
>   // Casting
>   static Klass* cast(Klass* k) {
>     assert(k->is_klass(), "cast to Klass");
>     return k;
>   }
>
> which begs the question: how can k->is_klass() not be true? And have 
> we lost anything by getting rid of these "casts"? There are additional 
> checks in instanceKlass::cast too
>
>  // Casting from Klass*
>   static InstanceKlass* cast(Klass* k) {
>     assert(k->is_klass(), "must be");
>     assert(k->oop_is_instance(), "cast to InstanceKlass");
>     return (InstanceKlass*) k;
>   }
>
> Is the need for the asserts obsolete in a NPG world?
>
> That aside in vframe.cpp:
>
> +  InstanceKlass*     k = m->method_holder();
>    tty->print_cr("frame( sp=" INTPTR_FORMAT ", unextended_sp=" 
> INTPTR_FORMAT ", fp=" INTPTR_FORMAT ", pc=" INTPTR_FORMAT ")",
>                  _fr.sp(),  _fr.unextended_sp(), _fr.fp(), _fr.pc());
>    tty->print("%s.%s", Klass::cast(k)->internal_name(), 
> m->name()->as_C_string());
>
> there is a Klass::cast that can be removed.
>
> Thanks,
> David
> -----
>
>>
>> This webrev has a lot of files but most of the changes are small.
>>
>> Open webrev at http://cr.openjdk.java.net/~coleenp/bug_8000725
>> <http://cr.openjdk.java.net/%7Ecoleenp/bug_8000725>
>>
>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8000725
>>
>> The changes were tested with JPRT, JCK, and other tests including ones
>> for sa-jdi.
>>
>> Thanks, Harold