On Fri, Jun 15, 2018 at 10:36 PM, Chris Plummer
<chris.plum...@oracle.com> wrote:
> On 6/15/18 10:31 AM, Thomas Stüfe wrote:
>>
>> Coleen, Chris,
>>
>> thanks for the review. Please find the latest webrev:
>>
>> Delta:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.01-to-02/webrev/index.html
>> Full:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.02/webrev/
>>
>> I changed the copyright dates, the format of the initializer lists (I
>> looked around and used what the most common denominator seemed to be -
>> still, Coleen, should you ever dig up that style guide of yours, that
>> would be valuable), and initialized CmdLine::_cmd to line as Coleen
>> sugggested.
>>
>> @Chris
>>>
>>> I'm not certain why you added default field initializers for
>>> CmdLine::CmdLine when each field is explicitly initialized within the method
>>> body.
>>
>> See Coleens response. I usually prefer it that way if the constructor
>> is not extremely obvious, to prevent missing initializations should
>> the constructor body evolve. C++ compiler should optimize that away
>> anyway.
>
> I tend to think the opposite. Using the initializer list in this way would
> lead me to think that the field was not initialized in the constructor body,
> or at the very least there was a path where it might not be initialized. I
> don't see how this helps when the constructor is not obvious. Is the point
> of the initializer (in this case) solely to have the code document which
> fields get initialized in the constructor?
>
For me, it is a safety valve against missing initialization. Consider this:
class Parser { const char* p; .. };
Parser(some input) {
// complicated stuff happening, p gets initialized in all paths. Ok.
}
evolves to this:
Parser(some input) {
// more complicated stuff, on one path p may not get initialized.
}
if p does not get initialized, its value is essentially random, at
least in relase builds. Especially if Parser is placed on the stack,
since p then is mapped to whatever happened to be at this stack slot
before.
So, by always initializing it to NULL:
Parser(some input) : p(NULL) {
// complicated stuff happening, p gets initialized in all paths
}
from the beginning, I get reproducable errors. Using p == NULL will
crash, using p == random-and-maybe-readable-pointer may do random
things or even work out of accident.
Thanks, Thomas
> thanks,
>
> Chris
>
>> Thanks for the reviews,
>>
>> Best Regards, Thomas
>>
>>
>> On Fri, Jun 15, 2018 at 1:53 PM, <coleen.phillim...@oracle.com> wrote:
>>>
>>>
>>> On 6/15/18 1:04 AM, Chris Plummer wrote:
>>>
>>> Hi Thomas,
>>>
>>> I'm not certain why you added default field initializers for
>>> CmdLine::CmdLine when each field is explicitly initialized within the
>>> method
>>> body.
>>>
>>>
>>> I think the C++ optimizer would elide any unnecessary initializations and
>>> it
>>> seems safe in case new fields are added, although
>>>
>>> 38 : _cmd(NULL)
>>>
>>>
>>> could be initialized as _cmd(line).
>>>
>>> Can I bikeshed and say I don't like the style where the , are at the
>>> beginning of the lines? We don't use this style in other places, that
>>> I've
>>> noticed, and I really don't like reading the punctuation at the beginning
>>> of
>>> the line. If I could find our open source wiki for the coding style
>>> among
>>> the broken openjdk wiki pages, I would see if the punctuation rule is
>>> there.
>>> One initializer per line is ok.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> A number of copyrights need updating.
>>>
>>> The rest of the changes look good.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 6/14/18 1:38 AM, Thomas Stüfe wrote:
>>>
>>> Hi all,
>>>
>>> (I moved the issue to target release 12 - if we get this through the
>>> review quick, fine, but no reason to add pressure)
>>>
>>> some more background: The reason for these cleanups is that I later
>>> plan to add a new sub command which allows us to run multiple commands
>>> in one safepoint (see [1] for more details and discussions). Since
>>> this will require tweaking the parse process a bit, I try to loosen up
>>> the coding before adding complexity again.
>>>
>>> Please find here the new webrev:
>>>
>>> Delta:
>>>
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00-to-01/webrev/
>>> Full:
>>>
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.01/webrev/
>>>
>>> What changed:
>>>
>>> Aesthetics:
>>> - I reordered member init lists to member order, as Coleen did ask. I
>>> also reform them as lists, just to increase readability.
>>> - For a number of inline methods, I aligned their implementations so
>>> that they too would be better readable and it would be clearer which
>>> methods are const
>>> - Moved DCmdFactory::_DCmdFactoryList up to the other static members.
>>>
>>> Functional:
>>> - Added initializer list for CmdLine
>>> - Added missing initializations for DCmdArgIter members _key_addr,
>>> _key_len, _value_addr, _value_len, as Coleen suggested
>>> - constified DCmdInfo too - since this is an immutable value holder,
>>> all members can be const.
>>> - also in DCmdInfo, changed JavaPermission permission() to const
>>> JavaPermission& permission() to save a bit unnecessary copying.
>>> - Fixed initialization of DCmdArgumentInfo::_position as Serguei and
>>> Chris requested
>>> - Removed default implementation of DCmd::name and DCmd::description.
>>> There is no good reason to provide one - all child classes should (and
>>> do) provide an implementation. I rather have linkage errors if someone
>>> forgets to add one.
>>> - Removed default implementations for DCmdWithParser::name/description
>>> - same reasoning as above
>>> - Removed DCmdWithParser::permission(), it is already provided by the
>>> base class.
>>> - Removed DCmdWithParser::num_arguments() since childs of
>>> DCmdWithParser will always have arguments and hence should implement
>>> this function.
>>> - Made all functions in DCmdFactoryImpl non-virtual. There is no point
>>> in allowing to override them.
>>>
>>> --
>>>
>>> Notes:
>>>
>>> I see the following commands do not implement "permission" and hence
>>> rely on the default impl of DCmd::permission, which returns a
>>> permission structure { NULL, NULL, NULL } - is that okay and intended?
>>>
>>> JMXStopRemoteDCmd
>>> JMXStartLocalDCmd
>>> JMXStartRemoteDCmd
>>> TouchedMethodsDCmd
>>> ClassStatsDCmd
>>> RunFinalizationDCmd
>>> SystemGCDCmd
>>> VMUptimeDCmd
>>> HelpDCmd
>>>
>>> I am not sure where and how these permissions are tested.
>>>
>>> Thinking through this further:
>>>
>>> Of the 47 child classes of DCmd, 37 seem to return
>>> "java.lang.management.ManagementPermission", name "monitor". So, we
>>> have this function 37 times:
>>>
>>> static const JavaPermission permission() {
>>> JavaPermission p = {"java.lang.management.ManagementPermission",
>>> "monitor", NULL};
>>> return p;
>>> }
>>>
>>> Would it then not make sense to make this the default implementation
>>> for DCmd::permission() - instead of the NULL permission - and only
>>> require child DCmds to provide a permission if it differs from this
>>> default? We could then loose all 37 methods.
>>>
>>> ---
>>>
>>> Similar thoughts for "static DCmd impact()"
>>>
>>> which is not provided by:
>>>
>>> JMXStatusDCmd
>>> JMXStopRemoteDCmd
>>> JMXStartLocalDCmd
>>>
>>> which thus return the default impact - DCmd::impact() - which returns
>>> "Low".
>>>
>>> Here I think we should either remove the default implementation and
>>> force all commands to provide an implementation or stay with the
>>> default "Low" implementation and remove all 20+ functions in child
>>> classes which also return "Low".
>>>
>>> --
>>>
>>> Finally, some thoughts for possible future cleanups:
>>>
>>> GenDCmdArgument internally uses C-heap to hold copies of strings
>>> (DCmdArgument<char*> and DCmdArgument<StringArrayArgument>). I wonder
>>> whether we could use resource area instead. Then we could get rid of
>>> all cleanup code (GenDCmdArgument::cleanup(),
>>> GenDCmdArgument::destroy_value()) etc. And if one agrees that parsing
>>> is only done once and the commands are not reused (which is the case
>>> currently) we also may get rid of DCmdArgument::reset() and friends.
>>> Coding would become a lot easier.
>>>
>>> --
>>>
>>> IMHO the way object- und template based polymorphy is mixed is a bit
>>> of a mismatch here and it complicates things a bit.
>>>
>>> For instance, Dcmd has above mentioned static functions which return
>>> immutable attributes for the commands. It is "overwritten"
>>> template-style by redefining the same static functions in derived
>>> classes and called via DCmdFactoryImpl<command class>.
>>>
>>> Maybe it is just me, but I find this a bit roundabout for the problem
>>> it tries to solve. Also, it is really difficult to parse for modern
>>> IDEs - e.g. my CDT is unable to tell me who calls "DCmd::permission()"
>>> since the relation between DCmd::permission() and XXXCmd::permission()
>>> is just by name. Also, why pay for the text size of 47 just slightly
>>> different template definitions of DCmdFactory<xxx> where one class
>>> would be enough?
>>>
>>> The mis-fit between templates and object polymorphism also shows up as
>>> weird artifacts. E.g. we have 27 variants of this function:
>>>
>>> int XXXXDCmd::num_arguments() {
>>> ResourceMark rm;
>>> XXXDCmd* dcmd = new SetVMFlagDCmd(NULL, false);
>>> if (dcmd != NULL) {
>>> DCmdMark mark(dcmd);
>>> return dcmd->_dcmdparser.num_arguments();
>>> } else {
>>> return 0;
>>> }
>>> }
>>>
>>> - so, we want to implement "static XXXDCmd::num_arguments", which -
>>> being static - has to create an object of the same type to ask it its
>>> number of arguments.
>>>
>>> Maybe we could un-templify the command classes at some point to
>>> simplify all this?
>>>
>>> I think we need two things, a class to hold meta information about the
>>> command - name, description, argument types etc - which needs to be
>>> available at startup. And then, we need a second thing to hold runtime
>>> context: the concrete argument values the user gave us, the
>>> outputStream* pointer, the execute() function, maybe parser state...
>>>
>>> I think this may be expressed simpler and more efficiently with simple
>>> C++ objects and inheritance, without templates. We could also shrink
>>> the code quite a bit. And my CDT would find who calls "permissions()"
>>> :)
>>>
>>>
>>> Best Regards, Thomas
>>>
>>> -------
>>>
>>> [1]
>>>
>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023673.html
>>>
>>>
>>> On Wed, Jun 13, 2018 at 3:41 PM, Thomas Stüfe <thomas.stu...@gmail.com>
>>> 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
>>>
>>>
>>>
>>>
>>>
>
>
