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

[Chris Plummer]

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
>>>>
>>>>