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