On 8/01/2015 1:59 AM, Jaroslav Bachorik wrote:
On 7.1.2015 02:31, David Holmes wrote:
On 17/12/2014 8:19 PM, Jaroslav Bachorik wrote:
Please, review the following change.
Issue : https://bugs.openjdk.java.net/browse/JDK-8067447
Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.00
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 ManagedFlags class and the codebease is adjusted to use this
class.
Not clear to me what this is addressing exactly - do we really need
platform specific variants of "set flag" ??
It has just been moved from the corresponding attachListener_<os>.cpp
files. And it is 'pd_set_flag' what, I suppose, means "platform dependent"?
Yes it does and it mostly made sense inside the already pd
attachListener implementations, but it isn't obvious to me that it makes
sense in the ManagedFlag context. It is the choice about whether the
flag can be changed that is "pd" not the actual setting and those
choices are inherent to the attachListener mechanism they are not
inherent to ManagedFlags - so this refactoring seems wrong to me. What
exactly is ManagedFlag supposed to represent?
All the new code seems incorrect:
jint ManagedFlags::pd_set_flag(const char* flag_name, const char*
flag_value, Flag::Flags origin, outputStream* out) {
out->print_cr("flag '%s' cannot be changed", op->arg(0));
return JNI_ERR;
};
op->arg(0) comes from the original code where op was an
AttachOperation*. Here is should be using flag_name.
Correct. Slipped through and then replicated :/
And obviously never compiled. RFRs should be for tested code.
David
-----
Updated webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.01
-JB-
David
Thanks,
-JB-
