Request for review: 7132690 InstanceKlass:_reference_type should be u1 type

[Dean Long]

I agree with Vitaly.  You can find existing examples in the code:

   interpreter/rewriter.cpp:        assert(cache_index == 
(u1)cache_index, "index overflow");

You could also assert that reference_type == t after the assignment.

dl

On 1/24/2012 7:19 PM, Jiangli Zhou wrote:
> Hi Vitaly,
>
> Thanks for catching it! A range check seems to be more explicit in 
> this case:
>
> http://cr.openjdk.java.net/~jiangli/7132690/webrev.02/
>
> Thanks,
>
> Jiangli
>
> On 01/24/2012 04:17 PM, Vitaly Davidovich wrote:
>>
>> I think you'll be fully covered (including negative values) by doing 
>> the following instead of hard coding max value in the assert:
>>
>> assert(t == (u1)t, "doesn't fit")
>>
>> Sent from my phone
>>
>> On Jan 24, 2012 6:33 PM, "Jiangli Zhou" <jiangli.zhou at oracle.com 
>> <mailto:jiangli.zhou at oracle.com>> wrote:
>>
>>     The updated webrev is :
>>     http://cr.openjdk.java.net/~jiangli/7132690/webrev.01/
>>     <http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.01/>
>>
>>     Thanks,
>>     Jiangli
>>
>>     On 01/24/2012 02:54 PM, Jiangli Zhou wrote:
>>>     Hi Vitaly,
>>>
>>>     An assert in the setter probably is a good idea. I should have
>>>     added it when making the change. Thanks for the comments!
>>>
>>>     Thanks,
>>>     Jiangli
>>>
>>>     On 01/24/2012 02:13 PM, Vitaly Davidovich wrote:
>>>>
>>>>     Hi Jiangli,
>>>>
>>>>     This is probably overly paranoid so feel free to ignore, but
>>>>     should the setter in InstanceKlass assert that the passed in
>>>>     ReferenceType fits into a u1 instead of silently narrowing it?
>>>>     Or change the setter to take a u1 and make caller do the cast?
>>>>     This would prevent someone defining another member of the enum
>>>>     with an explicit value that doesn't fit into u1.  Like I said,
>>>>     paranoia ... :)
>>>>
>>>>     Thanks
>>>>
>>>>     Sent from my phone
>>>>
>>>>     On Jan 24, 2012 3:06 PM, "Jiangli Zhou"
>>>>     <jiangli.zhou at oracle.com <mailto:jiangli.zhou at oracle.com>> wrote:
>>>>
>>>>         Hi,
>>>>
>>>>         Please review the change for 7132690:
>>>>
>>>>         http://cr.openjdk.java.net/~jiangli/7132690/webrev.00/
>>>>         <http://cr.openjdk.java.net/%7Ejiangli/7132690/webrev.00/>
>>>>
>>>>         It changes InstanceKlass::_reference_type from
>>>>         ReferenceType to u1. The ReferenceType is defined as an
>>>>         enum with 6 values
>>>>         (src/share/vm/utilities/globalDefinitions.hpp). The change
>>>>         saves 4-byte for each class.
>>>>
>>>>         Tested with runThese and ute vm.quick.testlist. Ran jprt.
>>>>
>>>>         Thanks,
>>>>
>>>>         Jiangli
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120124/7bd69106/attachment.html