Hi Serguei, On Thu, Jun 14, 2018 at 1:11 AM, serguei.spit...@oracle.com <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? >
Yes, I was surprised that this even compiled, but it did. I only did catch it because CDT complained. > > > 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)? > Yes, this is a real bug. Sorry, I'll fix it and comb the rest of the change again. Thanks, Thomas > 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 > >
