RFR 8067447: Factor out the shared implementation of the VM flags manipulation code

Staffan Larsen staffan.larsen at oracle.com
Thu Feb 5 13:55:10 UTC 2015


Jaroslav,

This looks good, just a few small comments:

writableFlags.hpp:

L25 #ifndef WritableFlags_HPP
L26 #define WritableFlags_HPP
L96 #endif /* WritableFlags_HPP */
Should be SHARE_VM_SERVICES_WRITABLEFLAG_HPP

L32: I don’t like inverted logic such as “NO_ERR”. I would prefer “SUCCESS” instead.

writableFlags.cpp:

L25: #include statements should be in alphabetical order if possible
L166: (nit) missing empty line
L155: I notice the Flag class uses “writeable” (with an ‘e’) and this class uses “writable” (without ‘e’) - My english isn’t good enough to tell if there is any difference.
L194: Unused variable “succeed"


Thanks,
/Staffan

> On 4 feb 2015, at 17:55, Jaroslav Bachorik <jaroslav.bachorik at oracle.com> wrote:
> 
> Hi, this is a round 2 review of the proposed solution.
> 
> Issue : https://bugs.openjdk.java.net/browse/JDK-8067447
> Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.02
> 
> This patch is a precursor for implementing https://bugs.openjdk.java.net/browse/JDK-8054890 which itself is a part of JEP-228 (https://bugs.openjdk.java.net/browse/JDK-8043764).
> 
> Here, the code related to manipulating JVM flags is extracted to a separate WritableFlags class and the codebease is adjusted to use this class.
> 
> I didn't touch the original (nonstandard) handling of the DTrace specific flags in the Solaris specific attachListener.cpp implementation to keep the change simple and focused.
> 
> Thanks,
> 
> -JB-



More information about the serviceability-dev mailing list