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

[Staffan Larsen]

> On 6 feb 2015, at 12:54, Jaroslav Bachorik <jaroslav.bachorik at 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.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-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150206/28d28a30/attachment-0001.html>