Hi David, > That can be fixed using a no-arg > constructor for static initialization and adding a private setType method > used for the real initialization.
I uploaded new webrev. Is it okay? http://cr.openjdk.java.net/~ysuenaga/JDK-8190837/webrev.01/ > I'm not at all clear why we need the tXxx and T_XXX forms? The former can be > obtained from the latter. I agree with you, but it is difficult. For example, [1] has a lot of lines which use BasicType. I had a lot of compile errors when I removed getTXxx methods from BasicType... Thanks, Yasumasa [1] http://hg.openjdk.java.net/jdk/hs/file/5a449dbca6d0/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/Bytecodes.java#l560 2017-11-29 16:01 GMT+09:00 David Holmes <[email protected]>: > On 29/11/2017 4:19 PM, Jini George wrote: >> >> Hi Chris, >> >> Thank you for raising this. I agree it is disruptive, but we are trying to >> address the issue of frequent SA breakages with hotspot changes, and the >> causes of these breakages. One of the reasons is the redefinition of >> constants, which is extremely error prone. There have been multiple cases >> where the changes get done in hotspot and the corresponding changes get >> inadvertently missed out in SA. And this does not get caught, sometimes, for >> months. I believe that the switch case statements conversion to if-else >> statements is not a heavy price to pay for avoiding errors like these. > > > I agree it is good to ensure this always matches the VM. I also agree it is > unfortunate we lost the ability to keep the switch statements - so be it. > I'm more concerned that the BasicType static fields are no longer final > (that may raise parfait warnings!). That can be fixed using a no-arg > constructor for static initialization and adding a private setType method > used for the real initialization. > > I'm not at all clear why we need the tXxx and T_XXX forms? The former can be > obtained from the latter. And with the change to use the getTXxx() functions > I think we can actually do away with all the tXxx static fields. The > getTXxx() functions can be implemented as "return T_XXX.getType(); and the > intToBasicType() function can also examine getType(). The name could be > stored as a field, set at construction time. Just a thought. :) > > Thanks, > David > > >> Thanks! >> - Jini. >> >> On 11/29/2017 7:56 AM, Chris Plummer wrote: >>> >>> 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 <[email protected]>: >>>>> >>>>> 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 >>>>>>> >>>>>>> >>> >
