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.
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.
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.
Still a really cool test here!
Thumbs up. Your call on whether to tweak the test.
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
