Re: RFR(s): 8204958: Minor cleanups for the diagnostic framework

[Chris Plummer]

On 6/13/18 9:53 PM, serguei.spit...@oracle.com wrote: On 6/13/18 21:19, Chris Plummer wrote: On 6/13/18 6:11 PM, serguei.spit...@oracle.com wrote: On 6/13/18 16:44, Chris Plummer wrote: On 6/13/18 4:11 PM, serguei.spit...@oracle.com wrote: Hi Thomas, I looks good to me. A couple of minor comments. http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/src/hotspot/share/services/diagnosticArgument.hpp.udiff.html - void value_as_str(char *buf, size_t len) { return to_string(_value, buf, len);} + void value_as_str(char *buf, size_t len) const { to_string(_value, buf, len);} I'm puzzled a little bit. How did the value_as_str() returned something before if the function has void return type? I guess because to_string() returns void. I just did a little experiment and the following compiles just fine: void return_void() {   return; } void return_void2() {   return return_void(); } Thanks, Chris. I thought about the same. http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/src/hotspot/share/services/diagnosticFramework.hpp.frames.html 170 DCmdArgumentInfo(const char* name, const char* description, const char* type, 171 const char* default_string, bool mandatory, bool option, 172 bool multiple, int position = -1) 173 : _name(name), _description(description), _type(type) 174 , _default_string(default_string), _mandatory(mandatory) 175 , _option(option), _multiple(multiple), _position(-1) {}   Would it be more safe for the _position initialization to be: _position(position)? I think it's actually a bug and should be _position(position). Agreed. Are you Okay with this fix? I think it should be _position(position). If the argument is not specified, it will get the default of -1 as specified in the prototype, which is what you want. I asked not about this part but about the whole webrev. Sorry, it was not clear. Yes, but I'm always a little hesitant about constifying (ok, so that's probably not really a word) everything that seems to logically be const. I've been bit too much in the past with having to cast away the const because it was not well thought out, but I think it looks pretty safe in the work that Thomas has done. Chris Thanks, Serguei thanks, Chris Thanks, Serguei Chris Thanks, Serguei On 6/13/18 06:41, Thomas Stüfe wrote: Hi all, while working on jcmd I saw some minor cleanup possibilities. Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8204958 Most of them are const correctness and using initializer lists instead of java-style member initialization in these C++ classes. Other changes: - Added a ResourceMark into the DCmd::parse_and_execute() loop - removed DCmdFactory::create_global_DCmd() (and following that, DCmdFactory::create_Cheap_instance()) since I did not find anyone using that function. - Also in DCmdFactory, I removed a number of setters which were never called and for attributes which should be immutable: hidden, flags, enabled. This is really a rather light cleanup. It did not touch any larger issues. Thank you, Thomas