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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/Opts.td:12
+                     Values<"binary">;
+def output_format_alias : JoinedOrSeparate<["-"], "O">,
+                          Alias<output_format>;
----------------
LLVM style is to name this `O`, so that you can find it as `OPT_O`.


================
Comment at: llvm/tools/llvm-objcopy/Opts.td:22
+                              Alias<add_gnu_debuglink>;
+defm to_remove : Eq<"remove-section">,
+                 MetaVarName<"section">,
----------------
LLVM style would be to name this `remove_section`.


================
Comment at: llvm/tools/llvm-objcopy/Opts.td:25
+                 HelpText<"Remove <section>">;
+def to_remove_alias : JoinedOrSeparate<["-"], "R">,
+                      Alias<to_remove>;
----------------
LLVM style would be to name this `R`.


================
Comment at: llvm/tools/llvm-objcopy/Opts.td:33
+                 HelpText<"Remove all but <section>">;
+def only_keep_alias : JoinedOrSeparate<["-"], "j">,
+                      Alias<only_keep>;
----------------
LLVM style would be to name this `j`.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D44236