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

[coleen.phillimore at oracle.com]

On 4/25/19 4:19 PM, serguei.spitsyn at 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.spitsyn at 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
>>>
>>>
>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190425/d617fc6d/attachment-0001.html>