Thank you Coleen. I pushed. The document reads a bit outdated in places:
<quote>Be sparing with templates.</quote> I think that ship has sailed :P Best Regards, Thomas On Sat, Jun 16, 2018 at 12:07 AM, <coleen.phillim...@oracle.com> wrote: > > > On 6/15/18 4:52 PM, Thomas Stüfe wrote: >> >> 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 >> > > I agree with Thomas and I think the change looks good. Thank you for > humoring me with the punctuation. It doesn't seem to be here. > > https://wiki.openjdk.java.net/display/HotSpot/StyleGuide > > Thanks, > Coleen > > >> >> >> >> >> >> >>> 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 >>>>> >>>>> >>>>> >>>>> >>>>> >>> >
