On Thu, Jun 14, 2018 at 6:19 AM, Chris Plummer <chris.plum...@oracle.com> 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. >
Oh, good catch. Thank you Chris, I'll fix this. I'll prepare a new webrev. ..Thomas > 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 > > > > >
