Hi Jaroslav,
// a writable flag
Still a few changes needed in writeable.hpp and writeable.cpp
But no need for a new webrev.
Thanks,
David
On 9/02/2015 8:48 PM, Jaroslav Bachorik wrote:
On 9.2.2015 00:13, David Holmes wrote:
Hi Jaroslav,
On 6/02/2015 9:54 PM, Jaroslav Bachorik 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
src/share/vm/services/writableFlags.cpp
precompiled.hpp must be first in the include list.
done.
73 int WritableFlags::set_uintx_flag(const char* name, uintx value,
Flag::Flags origin, FormatBuffer<80>& err_msg) {
74 if (strncmp(name, "MaxHeapFreeRatio", 17) == 0) {
Strikes me that this flag specific validation does not belong in this
logic (nor does it belong where it currently is today). Maybe the work
being done on argument validation will allow this to be cleaned up in
the near future?
i hope so. slowly getting there - instead of 3 different places
performing this validation we have 2 now.
154 // only writable flags are allowed to be set
155 if (f->is_writeable()) {
This tells me you maybe made the wrong choice for the spelling issue :)
If we already have writeable associated with flags then maybe stick with
it?
renamed to 'writeable'
http://cr.openjdk.java.net/~jbachorik/8067447/webrev.04
-JB-
Otherwise seem okay.
Thanks,
David
-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-
