> On 6 feb 2015, at 12:54, Jaroslav Bachorik <jaroslav.bacho...@oracle.com> > wrote: > > On 6.2.2015 06:16, David Holmes wrote: >> On 6/02/2015 1:17 AM, Jaroslav Bachorik wrote: >>> On 5.2.2015 14:55, Staffan Larsen wrote: >>>> 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. >>> >>> They should be equal in meaning. According to GooglBattle [1] the form >>> 'writable' is far wider spread but I would leave the decision to a >>> native speaker. >>> >>> [1] >>> http://www.googlebattle.com/index.php?domain=writeable&domain2=writable&submit=Go! >>> >>> >>> >>>> L194: Unused variable “succeed" >>> >>> The rest of the comments will be addressed. I will wait for eg. David to >>> comment on 'writable' vs. 'writeable' before regenerating the webrev. >> >> Take your pick. Both are used in the hotspot sources eg globals.hpp >> contains is_writeable() and check_writable() :( >> >> It is on my TODO list to review this ASAP. > > Taffan's comments addressed - > http://cr.openjdk.java.net/~jbachorik/8067447/webrev.03 > <http://cr.openjdk.java.net/~jbachorik/8067447/webrev.03>
(First time I’m called that :-) ) Looks good. I realize now that precompiled.hpp is special and should be included before anything else. I don’t need to see the fix before you push. Thanks, /Staffan > > -JB- > >> >> David >> >> >> >>> -JB- >>> >>>> >>>> >>>> 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-
