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

[David Holmes]

On 25/01/2012 2:41 PM, Vitaly Davidovich wrote:
> Hi Jiangli,
>
> I think doing the comparison via the cast and == check to make sure it
> "roundtrips" is better than explicit range check because this way you
> don't have to change the assert if you change the width of the type
> (e.g. using u2 instead of u1 would require updating the range).  Plus as
> Dean mentioned, it looks like the cast version is already used in the
> codebase.

Plus the range check assumes that the enum will never have negative 
values (not likely here but why preclude it).

David

> Cheers
>
> Sent from my phone
>
> On Jan 24, 2012 10:19 PM, "Jiangli Zhou" <jiangli.zhou at oracle.com
> <mailto:jiangli.zhou at oracle.com>> 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
>>>>
>>>
>>
>