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

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

Thanks, Coleen! Serguei On 4/26/19 11:39, coleen.phillim...@oracle.com wrote: Looks good to me. Coleen On 4/26/19 2:31 PM, serguei.spit...@oracle.com wrote: Hi Dan, Thank you for re-review! I'll fix the spaces at the end, thanks. Thanks, Serguei On 4/26/19 08:27, Daniel D. Daugherty wrote: On 4/25/19 10: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/   src/hotspot/share/runtime/globals.hpp     No comments. test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java     No comments. test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java     L100:     // So, this redefinition will add it back which is expected to work.         There's a space at the end of this comment line. jcheck may complain.     L132:     // So, this redefinition will deleate it back which is expected to work.         Typo: s/deleate it back/delete it/         There's a space at the end of this comment line. jcheck may complain. Thumbs up. Dan This implements the suggestions:  VMDeprecatedOptions.java:   - moved the option to the deprecated non-alias flags section  TestAddDeleteMethods.java:   - removed confusion in redefinition string names and added comments recommended by Dan   - always list methods in order: foo, publicFoo, finalFoo, staticFoo 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 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. :) 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