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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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?



================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:10-12
+#include "llvm/Option/Arg.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/Option.h"
----------------
These headers are in the wrong place in the list.


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


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:137
+  void SplitDWOToFile(const Reader &Reader, StringRef File, ElfType OutputElfType) const;
+};
 
----------------
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?




================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:152
 
-static ElfType OutputElfType;
-
-std::unique_ptr<Writer> CreateWriter(Object &Obj, StringRef File) {
+std::unique_ptr<Writer> Config::CreateWriter(Object &Obj, StringRef File, ElfType OutputElfType) const {
   if (OutputFormat == "binary") {
----------------
clang-format and elsewhere.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:347
+
+void ObjcopyDriver(ArrayRef<const char *> ArgsArr) {
+  ObjcopyOptTable T;
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:349
+  ObjcopyOptTable T;
+  unsigned MAI, MAC;
+  auto InputArgs = T.ParseArgs(ArgsArr, MAI, MAC);
----------------
Please don't abbreviate these two variables - I have no idea what "MAI" and "MAC" are.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:408
+
+  return 0;
 }
----------------
I don't think return 0 is necessary in main?


Repository:
  rL LLVM

https://reviews.llvm.org/D44236