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

[Thomas Stüfe]

Hi Chris,

On Thu, Jun 14, 2018 at 7:01 AM, Chris Plummer <chris.plummer at oracle.com> wrote:
> On 6/13/18 9:53 PM, serguei.spitsyn at oracle.com wrote:
>
> On 6/13/18 21:19, Chris Plummer wrote:
>
> On 6/13/18 6:11 PM, serguei.spitsyn at oracle.com wrote:
>
> On 6/13/18 16:44, Chris Plummer wrote:
>
> On 6/13/18 4:11 PM, serguei.spitsyn at 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.
>

Constifying makes the code really a lot more readable, especially to
outsiders like me. Since it conveys purpose: which variables are
supposed to change, which are immutable attributes?

I usually constify in sections, where the interfaces to the outside
are slim and one has to cast away the const as seldom as possible, if
at all. You are right, when done wrong it can get annoying.

..Thomas

> 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
>
>
>
>
>
>
>