[PATCH] D44236: [llvm-objcopy] Switch over to using TableGen for parsing arguments

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

In https://reviews.llvm.org/D44236#1031086, @jhenderson wrote:

> Like the others who have commented, it seems to me like the execute() function should be a member of a driver class (or stand-alone, since we don't really need a class here), rather than a configuration class. A configuration describes how to do something, but I don't think doing it itself feels right to me (for a slightly abstract example, think configuration files for an application - the executable does the work, the config file just allows controlling what is done).
>
> Is there a particular reason you don't want to follow LLD's approach of one global configuration variable? Alternatively making it a member of an "ObjcopyDriver" class?


Making an ObjcopyDriver class is probably fine. Eventually I want the Config to be copyable so that multiple different configs can be used at the same time.



================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:54
+               HELPTEXT, METAVAR, VALUES)                                      \
+  OBJCOPY_##ID,
+#include "Opts.inc"
----------------
jhenderson wrote:
> Is there a particular reason you've gone with "OBJCOPY_<OPTION_NAME>" here? I think it's more common to do OPT_<OPTION_NAME>.
Eventually these will clash with the llvm-strip symbols for the same thing.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:137
+  void SplitDWOToFile(const Reader &Reader, StringRef File, ElfType OutputElfType) const;
+};
 
----------------
jhenderson wrote:
> compnerd wrote:
> > jakehehrlich wrote:
> > > alexshap wrote:
> > > > for what it's worth, it seems to me this is kind of strange that a struct Config has these methods / logic (i mean CreateWriter, CreateReader, SplitDWOToFile). I've just looked at LLD which also has some kind of Config internally, but there the Config is not loaded with this business logic.
> > > LLD uses a global Config object instead of warping it in a class like I have. I also don't like what I've done that much. The alternative that I see is to pass the Config in as an argument. For HandleArgs that would be pretty painful. I can do that for the others though, what do you think? Alternatively I can go though the burden of  Maybe I should just use a different name? The idea is that you want to be able to set and configure all these different values and then make the result happen. Each driver should set one of these up for each copy operation and it should execute it. Any other ideas?
> > I agree with @alexshap here, I don't think that the `Config` structure here should have anything in it other than the options.  It can be consumed by the tool to drive the tool as necessary.  I can understand the desire to share code between the tool, but, I think that standalone functions or a different class would make for a better home for this.
> > The alternative that I see is to pass the Config in as an argument. For HandleArgs that would be pretty painful.
> Why would it be painful? I don't see how it would be hard to extend this signature?
> 
> 
It would just mean typing "Config." a ton of times. I'm fine making it a parameter though. It also means *everything* needs to take Config as a parameter.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:347
+
+void ObjcopyDriver(ArrayRef<const char *> ArgsArr) {
+  ObjcopyOptTable T;
----------------
jhenderson wrote:
> I think I'd prefer the argument parsing split into a separate function from the driver function, since it's a distinct block of work, and will make the different stages of the program somewhat clearer. Ideally, I'd also just hoist the body of Config::Execute into this function then.
After I put Config::Execute in its own function (per the discussion above) that code will (literally in the next diff) be shared between the StripDriver and the ObjcopyDriver


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:349
+  ObjcopyOptTable T;
+  unsigned MAI, MAC;
+  auto InputArgs = T.ParseArgs(ArgsArr, MAI, MAC);
----------------
jhenderson wrote:
> Please don't abbreviate these two variables - I have no idea what "MAI" and "MAC" are.
Yeah fair, that was cargo-culted from other tools. They stand for "missing argument count" and "missing argument index" which is 100% not clear. I'll fix that.


Repository:
  rL LLVM

https://reviews.llvm.org/D44236