[PATCH] D50343: [llvm-objcopy] Add support for -I binary -B <arch>.

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:133
   StringRef InputFormat;
-  StringRef BinaryArch;
+  MachineInfo BinaryArch;
 
----------------
jhenderson wrote:
> rupprecht wrote:
> > jhenderson wrote:
> > > It looks to me like we are grouping things in CopyConfig by type, so this should probably be moved to before (or after) the StringRef block.
> > Done -- I added some comments to try to logically explain how this config is laid out, but I'm not very familiar with all of them... I'm open to suggestions here.
> I'd like @jakehehrlich to comment on these comments, if that's okay, as he may have a specific desire as to how this class is laid out.
To date I have had no reason or rhyme to how I've added these...in fact I'm not sure if I have even added the majority at this point. A pattern of grouping by type does seem to have formed organically however. Let's stick with it.

Long term ideals on how this should be laid out: 

1) The names should have a consistent method for naming them derived from the option name where possible. I don't think I've done a very good job of this and it isn't clear how to best solve this issue.
2) Grouping first by type and then by alphabetization is probably a good idea.  I'm pretty trash at finding things in alphabetized lists but I always know the basic type of thing I'm looking for. AddSection is the only thing I know of which currently violates this. (we should make it work the way Paul made SectionsToRename work).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:134-135
   StringRef InputFilename;
   StringRef OutputFormat;
   StringRef InputFormat;
 
----------------
Do we use these anywhere? I think I added them thinking about how I was going to use them to do exactly what this change does but embedding MachineInfo seems much nicer and convey's the same information. Maybe we should just do that?


Repository:
  rL LLVM

https://reviews.llvm.org/D50343