Hi Serguei,
Thanks for making these additional changes.
On 26/04/2019 12:57 pm, serguei.spit...@oracle.com wrote:
Hi Coleen and Dan,
Updated webrev is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.2/
This implements the suggestions:
VMDeprecatedOptions.java:
- moved the option to the deprecated non-alias flags section
Yep that's fine.
TestAddDeleteMethods.java:
- removed confusion in redefinition string names and added comments
recommended by Dan
- always list methods in order: foo, publicFoo, finalFoo, staticFoo
One nit:
39 import java.lang.Runnable;
java.lang.* is implicitly imported so we don't list any imports for
java.lang types.
Thanks,
David
-----
Thanks,
Serguei
On 4/25/19 2:41 PM, serguei.spit...@oracle.com wrote:
Hi Coleen,
Thank you a lot for looking at this!
On 4/25/19 2:18 PM, coleen.phillim...@oracle.com wrote:
On 4/25/19 4:19 PM, serguei.spit...@oracle.com wrote:
Hi Dan,
Thank you a lot fore reviewing this!
On 4/25/19 12:40, Daniel D. Daugherty wrote:
On 4/24/19 6:18 PM, serguei.spit...@oracle.com wrote:
Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8222934
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.1/
src/hotspot/share/runtime/globals.hpp
No comments.
test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java
L42: // deprecated class redefinition flags:
L43: {"AllowRedefinitionToAddDeleteMethods", "true"},
L44:
L45: // deprecated non-alias flags:
I think your new flag entry should have been added to the
"deprecated non-alias flags" section. You don't need to
call out that this is a "class redefinition" flag.
The reason for the "// deprecated alias flags (see also
aliased_jvm_flags):"
section (below what you changed) is because there is more
work to do for those flags.
Okay, I'm not very familiar with this test, will check how to change it.
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java
L94: public static String ADeleteStaticFoo =
This case is deleting both "staticFoo" and "finalFoo".
Is that what you really want? If so, then the test case
is misnamed.
I see your confusion here.
The ADeleteStaticFoo is used after the ADeleteFinalFoo.
So, the "finalFoo" has been already deleted before.
Then the ADeleteStaticFoo only deletes the "staticFoo".
The same was not the case for ADeleteFinalFoo.
It is because the redefinitions with ADeleteFoo and ADeletePublicFoo
are expected to be rejected with UOE.
L119 public static String BAddStaticBar =
This case is added "staticBar" and "finalBar". Is that what
you really want? If so, then the test case is misnamed.
This one is similar to the above.
The "finalBar" has already been added bythe BAddFinalBar redefinition.
Please, let me know if you are Okay with it as it is or prefer to
add a comment with clarification.
Still a really cool test here!
The test was initially written by Coleen (thanks, Coleen!)
I've spoiled it a little bit though. :)
Hi Serguei, You added a lot to it, which is taking me a while to
understand.
Why did you make class A inherit from Runnable?
In fact, nothing foxy.
It implements Runnable, not inherits. :)
There were two steps.
First was to decide if we there is a point to call methods in the
redefined classes A and B.
You did it with the in the original test version but you made
publicFoo to call others.
So, I decided that it is useful to make sure the methods are executed
well after redefinition.
Then I decided to use another class B for added methods.
Calling other methods from publicFoo did not work for me.
I had to generalize it with run() method and then made
classes A and B to implement Runnable to make it more clear.
Can you maintain order of the function declarations?
78 public static String ADeletePublicFoo =
finalFoo should be before staticFoo in this one.
Nice catch, thanks!
Will fix it in the webrev update.
Oh, now I see what Dan is talking about. In ADeleteStaticFoo,
finalFoo has already been deleted so you didn't want to also test
adding it back.
Right.
Thank you for enhancing the test. I guess it's good that it tests
the new option.
Thanks!
Serguei
Coleen
Thumbs up. Your call on whether to tweak the test.
I'll send a VMDeprecatedOptions related update later.
Thanks!
Serguei
Dan
Summary:
David, in review for
https://bugs.openjdk.java.net/browse/JDK-8222934 suggested:
1. I would have suggested to add "(Deprecated)" to the
description of the new flag in globals.hpp
2. The new flag should have been added to the deprecated VM
options tests.
3. The new test should run in both a positive and negative mode
so that it also checks that the new flag works.
The webrev above implements this suggestion.
Testing:
In progress: Submit mach5 run for the updated tests.
Thanks,
Serguei
