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

[harold seigel]

Hi David,

I don't see any value in the below assert.  The compiler would have 
already ensured that any input argument is already of type Klass.  If 
that isn't true then we would need to add asserts to every method 
verifying that its input arguments are really the type that they are 
declared to be.

    static Klass* cast(Klass* k) {
    *  assert(k->is_klass(), "cast to Klass"); *
       return k;
    }

For the asserts in InstanceKlass::cast(), note that the call to 
set_pool_holder() (from method.cpp) calls InstanceKlass::cast().  So the 
asserts will get exercised.  The C++ compiler statically checks the rest 
of the cases.

Thanks, Harold

On 10/29/2012 10:55 PM, David Holmes wrote:
> On 29/10/2012 10:38 PM, harold seigel wrote:
>> 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).
>
> Can you comment on the loss of asserts and the failure modes those 
> asserts were covering?
>
> David
>
>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20121102/b99d9da5/attachment.html