Hi Yasumasa,
On 29/11/2017 6:42 PM, Yasumasa Suenaga wrote:
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/
Minor nit: The private setType method should be defined after:
250 //-- Internals only below this point
but otherwise this looks exactly like I had envisaged. No need to see
updated webrev.
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...
I wasn't suggesting getting rid of the getTXxx methods just the tXxx
fields - as you have done.
Thanks,
David
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
