[PATCH] D113356: [llvm-objdump] -p: Dump PE header for PE/COFF

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/COFF/private-headers.yaml:69
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion:   0
----------------
Here and the other zero-valued fields, it might be advisable to use a non-zero value, to avoid any instances of variables not actually being assigned values properly (e.g. they were assigned to the wrong value/the wrong variable was assigned this variable's value etc).


================
Comment at: llvm/tools/llvm-objdump/COFFDump.cpp:42
+public:
+  COFFDumper(const llvm::object::COFFObjectFile *Obj) : Obj(Obj) {
+    if (Obj->getPE32Header())
----------------
MaskRay wrote:
> alexander-shaposhnikov wrote:
> > explicit
> > maybe pass const ref ? (e.g. nullptr is not a valid value for the argument)
> OK, I switched `printCOFFFileHeader` as well.
Looks like `explicit` is still needed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113356/new/

https://reviews.llvm.org/D113356