Re: RFS(S): 8222934: mark new VM option AllowRedefinitionToAddOrDeleteMethods as deprecated

[serguei.spit...@oracle.com]

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 by the 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. :) 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