[PATCH] D48904: [llvm-objdump] Add --archive-headers (-a) option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D48904#1152154, @paulsemel wrote:

> Added Jame's suggestions.
>  Totally agree with all your points James (except the error one, as it might break the whole behavior)


Could you explain more about why you think that the error might break behaviour?

Please also add a test as I described earlier:

> We need a test for combining -a with another switch, e.g. -d.



================
Comment at: tools/llvm-objdump/MachODump.cpp:78-86
 cl::opt<bool>
     llvm::ArchiveHeaders("archive-headers",
                          cl::desc("Print archive headers for Mach-O archives "
                                   "(requires -macho)"));
 
+cl::alias
+ArchiveHeadersShort("a", cl::desc("Alias for --archive-headers"),
----------------
As this is no longer a MachO-specific option, surely it makes more sense for this to be in llvm-objdump.cpp, with the rest of the switches?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2156
+  unsigned GID = GIDOrErr.get();
+  outs() << format("%-3d ", GID);
+
----------------
This should change as well, if you're not padding the first one.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2191
+static void DumpObject(ObjectFile *o, const Archive *a = nullptr,
+                       const Archive::Child *C = nullptr) {
   StringRef ArchiveName = a != nullptr ? a->getFileName() : "";
----------------
If you get a moment, could you either NFC change the variable names to all be capitalised. Either that, or 'C' should be 'c'


Repository:
  rL LLVM

https://reviews.llvm.org/D48904