[PATCH] D48810: [llvm-objdump] Add --file-headers (-f) option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Some inline comment nits, and one thing I noticed regarding the "not implemented" part.



================
Comment at: test/tools/llvm-objdump/file-headers-pe.test:9
+OptionalHeader:
+# Unfortunately, all those flags are mandatory to set AddressOfEntryPoint
+# All the values are randomly picked values, they can't interfer in what
----------------
those -> these

Also add a full stop at the end of the line.


================
Comment at: test/tools/llvm-objdump/file-headers-pe.test:10-11
+# Unfortunately, all those flags are mandatory to set AddressOfEntryPoint
+# All the values are randomly picked values, they can't interfer in what
+# we are testing here
+  SizeOfHeapCommit: 1024
----------------
All ... picked. They can't interfere ... here.

(i.e. remove extra "values", change comma to full stop, fix typo, add trailing full stop).


================
Comment at: test/tools/llvm-objdump/file-headers-pe.test:27
+  ImageBase: 0x12
+  AddressOfEntryPoint: 0x1234
+sections:
----------------
May be worth moving this particular field above the comment, since it is an important part of the test.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2135
+  if (!o->isELF() && !o->isCOFF()) {
+    outs() << "not implemented.\n";
+    return;
----------------
Hmmm... Should this use `report_error`? Or is not implemented normally reported on stdout?

Also, how easy is to get a test that hits this case? If it's simple, I think it should have one. If not, no need to write a complicated one.


Repository:
  rL LLVM

https://reviews.llvm.org/D48810