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

[Jiangli Zhou]

Hi Vitaly and David,

I've updated the webrev to do a == check via the cast.

http://cr.openjdk.java.net/~jiangli/7132690/webrev.02/

Thanks,

Jiangli

On 01/24/2012 11:44 PM, David Holmes wrote:
> 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
>>>>>
>>>>
>>>
>>