On Mon, 29 Jul 2024 09:39:07 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>>> One more thing that's troubling me. (Apologies it's now and not last week.) >>> >>> I was looking at the _filename.value().get() usage and finding it >>> uncomfortable, compared to the previous simple _filename.value() 8-) Harder >>> to remember and to read and understand. Maybe we can avoid the two >>> accessors, it really is just a char*. >>> >>> These additional argument types look like part of the framework which never >>> found an audience: MemorySizeArgument has one usage in >>> CompilationMemoryStatisticDCmd, NanoTimeArgument looks unused -- so the >>> two-accessor usage is only in once place until now? >>> >>> Adding FileArgument as another of these might be the wrong direction, as >>> these classes are so almost redundant. >>> >>> What if we didn't add FileArgument, and kept using <char*> for _filename >>> args/opts: >>> >>> Then in DCmdArgument<char*>::parse_value(), recognise a "FILE" argument >>> type and call Arguments::copy_expand_pid there, to set _value. >>> >>> Just seeing if we can cut down some of the complexity here, as Thomas >>> mentioned, it is already very complex for what it is! >>> >>> (There is also the to_string method which seemed like it would be useful >>> here, but it needs a buffer so is more complex than calling two >>> accessors... Another thing that seems to part of the framework that was >>> never much adopted.) >> >> IMHO for a functional addition we should follow the established pattern. >> Reworking the framework is certainly useful, but I would like it if we could >> get this done first (I intend to use it in other DCmds). >> >> And if we simplify this coding, we should think first about how to do this >> and what to solve. Things that come to mind: >> >> - overuse of template >> - The argument-type-by-template-division and the runtime "type" string >> argument (the third argument to DCmdArgument) seem redundant >> - the fact that we keep command metadata (which should be constant) together >> with command invocation data (values for arguments that are scoped to a >> single command invocation) in a single global structure, and then rewrite >> the latter every time we invoke a command. That is a strange concept and >> makes cleaning up temporary memory non-trivial >> - the fact that each new command takes a ton of boilerplate coding (Just see >> the many many repetitions in diagnosticCommand.cpp) >> - the fact that we use runtime-polymorphy, which is completely fine, but >> then all metadata information are "static". So in order to e.g. know how >> many arguments a command takes, you need to know the command c... > > Thanks Thomas @tstuefe - > > We're agreeing that some of this framework is overly complex, and that we > aren't going to simplify the framework in this change. > > But the more we adopt the obscure parts of the framework, the the harder it > will be to move away from it, so that's the reason for suggesting not > creating the FileArgument class. Use the simpler parts of this machine, with > some special cases where necessary, like a char* argument which happens to be > used for a FILEname (an input filename which gets %p substitution). > > The logic I don't follow is: > Using this complex mechanism because it exists, when it only has one? actual > usage. This seems to contradict the earlier max path len notes where it's > suggested not to use a pattern established by about 140 other usages. > > Apologies Sonia for dragging this out, still really pleased to get this > change happening. Hi @kevinjwalls, I reverted back to `char*` and modified parsing based on the type `FILE`. Really hope this reaches a consensus! ------------- PR Comment: https://git.openjdk.org/jdk/pull/20198#issuecomment-2256698014
