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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

This looks good to me. Thanks!

In https://reviews.llvm.org/D44236#1058119, @alexshap wrote:

> so maybe the following approach can also be considered: (deliberately presented in the reverse order to see how different pieces would fit together):
>
>   int main(int argc, char **argv) {
>       StringRef ToolName = path::filename(argv[0]);
>       Config Conf;
>       if (ToolName) == "objcopy")
>          Conf = parseObjcopyArgs(argc, argv);
>       else if (ToolName = "strip") 
>          Conf = parseStripArgs(argc, argv);
>       ExecuteObjcopy(Conf);
>       return 0;
>   }
>   
>   void ExecuteObjcopy(const Config &Conf) {
>        auto Reader = createReader(Conf.InputName);
>        if (Reader.isELF())
>            return ExecuteElfObjcopy(Conf, Reader);
>        if (Reader.isCoff())
>            return ExecuteCoffObjcopy(Conf, Reader);
>   }
>   
>   void ExecuteElfObjcopy(const Config &C, Reader &R) {
>         Object O;  // intermediate representation which we discussed with Jake and James
>         O.read(R);   
>         /* modify the object according to the config, similarly to the existing handleArgs */
>         .....
>         .....  
>         .....
>         auto W = createWriter(R.elftype(), C.OutputFormat, C.OutputFilename);
>         O.write(W);
>    } 
>
> }
>
> (seems to be a bit simpler, less inheritance, all the classes are "stateful")






================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:137
+  void SplitDWOToFile(const Reader &Reader, StringRef File, ElfType OutputElfType) const;
+};
 
----------------
compnerd wrote:
> jakehehrlich wrote:
> > 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.
> I'm not sure I understand why there is still so much "configuration" in the `CopyAction` here.  The flag state should be hoisted into the `Config` as that is not part of the "copy" operation.
> 
> Could you please share what your plan is with the additional machinery here?  It should be possible to do the work more simply (as @alexshap mentions).
These aren't necessarily in direct correspondence to flags; they are for objcopy but not for strip. For instance strip will set StripAll unless another strip option is used.  The idea is that this is information shared by every action that could occur on any input type or via any driver (objcopy or strip). I like Alex's idea though so I'm going to implement that.


Repository:
  rL LLVM

https://reviews.llvm.org/D44236