RFR (L) JDK-8231844: Enhance type signature characters in classfile_constants.h and improve the JVM to use type signature characters more consistently

[David Holmes]

Thanks Lois LGTM.

David

On 12/10/2019 5:32 am, Lois Foltan wrote:
> On 10/11/2019 8:29 AM, Frederic Parain wrote:
>>
>>> On Oct 10, 2019, at 21:15, David Holmes <david.holmes at oracle.com> wrote:
>>>>> ---
>>>>>
>>>>> src/hotspot/share/utilities/globalDefinitions.hpp
>>>>>
>>>>> I do not like this new method:
>>>>>
>>>>> +inline BasicType primitive_or_object(BasicType t) {
>>>>> +  assert(is_java_type(t), "%d", (int)t);
>>>>> +  return (is_reference_type(t) ? T_OBJECT : t);
>>>>> +}
>>>>>
>>>>> I found it's use in the code was far less clear than the original 
>>>>> code. I like to explicitly see where arrays and objects are being 
>>>>> treated the same (or differently).
>>>> This is in line with the change that I recently completed for 
>>>> JDK-8230505: Replace JVM type comparisons to T_OBJECT and T_ARRAY 
>>>> with call to is_reference_type.  Please refer to John's response to 
>>>> Coleen's review comment that I believe addresses your concern as 
>>>> well. 
>>>> https://mail.openjdk.java.net/pipermail/hotspot-dev/2019-September/039486.html 
>>>>
>>> I can't say that I agree. A simple query like is_reference_type() is 
>>> perfectly fine. But reducing all reference types to just T_OBJECT is 
>>> something that, to me, needs to be consciously chosen when a new 
>>> reference type is added. This approach could hide bugs as it will be 
>>> easy to miss locations that might new to handle the new reference 
>>> type differently. Just my 2c.
>> I agree with David here.
>>   1 - erasing all reference types (T_OBJECT, T_ARRAY, T_VALUETYPE) to 
>> T_OBJECT can be a source
>>       of confusion because each time we see T_OBJECT in the source 
>> code, we don’t know if it
>>       means T_OBJECT only or all reference types.
>>   2 - the name of the method “primitive_or_object()” is confusing too, 
>> its meaning is not obvious
>>       and it doesn’t sound like a conversion method (which it is)
>>
>> Would it be possible to have an internal T_REFERENCE type to clearly 
>> indicate erased reference types?
>> The use of a T_REFERENCE case in a switch statement would clearly 
>> indicate that the case applies
>> to all reference types.
>> Then the method could be renamed: as_primitive_or_REFERENCE() which I 
>> believe makes the conversion
>> more explicit.
>>
>> My 2c.
> 
> Thank you Fred for reviewing this change and your 2c!  Given both you 
> and David make valid points, it seems obvious more discussion is 
> warranted concerning specifically primitive_or_object().  So, I have 
> removed that from this change and have posted a new webrev at:
> 
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8231844.2/webrev/
> 
> The change is now just limited to changing the hard coded literals to 
> JVM_SIGNATURE_*.
> 
> Thanks,
> Lois
> 
>>
>> Fred
>>
>>
>>
>>
>