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
