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