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?
Can you maintain order of the function declarations?
78 public static String ADeletePublicFoo =
finalFoo should be before staticFoo in this one.
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.
Thank you for enhancing the test. I guess it's good that it tests the
new option.
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
