On 11/28/17 4:51 PM, Yasumasa Suenaga wrote:
Hi Chris,
2017-11-29 5:32 GMT+09:00 Chris Plummer <[email protected]>:
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
