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.bacho...@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-
