RFR (L) JDK-8231844: Enhance type signature characters in classfile_constants.h and improve the JVM to use type signature characters more consistently
Frederic Parain
frederic.parain at oracle.com
Fri Oct 11 12:29:13 UTC 2019
> 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.
Fred
More information about the hotspot-dev
mailing list