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.plum...@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.plum...@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.plum...@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 >>>>> >>>>> >
