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

[Lois Foltan]

Thanks again David for the review!
Lois

On 10/11/2019 7:04 PM, David Holmes wrote:
> 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
>>>
>>>
>>>
>>>
>>