RFR: 8190837: BasicType and BasicTypeSize should refer to HotSpot values

[Yasumasa Suenaga]

Hi Chris,

> What I'm asking is
> now that you've implemented it and seen the disruption to the switch
> statements (which I assume you and Jini were not aware of before embarking
> on this), is it still worth doing? It's not really that big of a deal to me.
> I just want to make sure it's been taken into consideration.

Yes. I think we should avoid error(s) in the future about changing
const value(s) in HotSpot.
They are difficult to catch on run-time.

So I send review request.


Thanks,

Yasumasa


2017-11-29 11:26 GMT+09:00 Chris Plummer <chris.plummer at oracle.com>:
> On 11/28/17 5:23 PM, Yasumasa Suenaga wrote:
>>
>> Hi Chris,
>>
>>> I understood the reason for getting rid of the case statements. I was
>>> just
>>> wondering if you weighed this code disruption vs. the value of what you
>>> are
>>> fixing.
>>
>> Jini has pointed it as below and I agree with him:
>>
>>
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
>> -------------
>> One point I want to make is that we have the
>> enum BasicTypeSize redefined in SA as public static final values, and
>> this makes it error prone when existing enum values change, just as in
>> this case. An ideal solution would be to include this in vmStructs.cpp
>> as a declare_constant() macro, and read this in SA with the
>> db.lookupIntConstant() method.
>> -------------
>
> Hi Yasumasa,
>
> Yes, I had read that and understand the point being made. What I'm asking is
> now that you've implemented it and seen the disruption to the switch
> statements (which I assume you and Jini were not aware of before embarking
> on this), is it still worth doing? It's not really that big of a deal to me.
> I just want to make sure it's been taken into consideration.
>
> thanks,
>
> Chris
>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> 2017-11-29 10:09 GMT+09:00 Chris Plummer <chris.plummer at oracle.com>:
>>>
>>> On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
>>>>
>>>> Hi Chris,
>>>>
>>>> 2017-11-29 5:32 GMT+09:00 Chris Plummer <chris.plummer at oracle.com>:
>>>>>
>>>>> Hi Yasumasa,
>>>>>
>>>>> This isn't code I know very well, and I'm not a Reviewer. Just a couple
>>>>> of
>>>>> observations.
>>>>>
>>>>> I wonder if the person who originally suggested this change realized
>>>>> the
>>>>> disruption it would have to existing switch statements. I'm not saying
>>>>> the
>>>>> change shouldn't be done for this reason, but it is something to at
>>>>> least
>>>>> consider.
>>>>
>>>> According to JLS, `case` label need to have constant expression.
>>>> We cannot set `static final` to the field which is set at initialize().
>>>>
>>>> https://docs.oracle.com/javase/specs/jls/se9/html/jls-14.html#jls-14.11
>>>
>>> I understood the reason for getting rid of the case statements. I was
>>> just
>>> wondering if you weighed this code disruption vs. the value of what you
>>> are
>>> fixing.
>>>>
>>>>
>>>>
>>>>> Your coding pattern for the following differs from the existing 200+
>>>>> instances of VM.registerVMInitializedObserver() calls already in place.
>>>>> I
>>>>> suggest you be consistent with existing code.
>>>>>
>>>>>     71   static {
>>>>>     72     VM.registerVMInitializedObserver(
>>>>>     73         (o, d) -> initialize(VM.getVM().getTypeDataBase()));
>>>>>     74   }
>>>>
>>>> This style has been used in JavaThreadsPanel.java .
>>>
>>> Ah, I missed that one case, but then it's one that you added. :)
>>>>
>>>> I like it because it is simple.
>>>>
>>>> I will change it to traditional style if other reviewer(s) suggest it.
>>>
>>> I think consistency is important.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>>
>>>>> On 11/27/17 11:49 PM, Yasumasa Suenaga wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Enum values in BasicType and BasicTypeSize are declared as const
>>>>> values. However, it makes error prone when existing enum values
>>>>> change.
>>>>> They should refer to HotSpot values via VMStructs.
>>>>>
>>>>> This issue has been pointed by Jini [1].
>>>>>
>>>>> I uploaded webrev for this issue. Could you review it?
>>>>>
>>>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.00/
>>>>>
>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> [1]
>>>>>
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-October/021965.html
>>>>>
>>>>>
>