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 Fred!
Lois

On 10/14/2019 11:35 AM, Frederic Parain wrote:
> Looks good to me.
> Thank you!
>
> Fred
>
>
>> On Oct 11, 2019, at 15:32, Lois Foltan <lois.foltan at oracle.com> 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
>>>
>>>
>>>
>>>