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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objdump/COFFDump.cpp:42
+public:
+  COFFDumper(const llvm::object::COFFObjectFile *Obj) : Obj(Obj) {
+    if (Obj->getPE32Header())
----------------
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.


================
Comment at: llvm/tools/llvm-objdump/COFFDump.cpp:81
+
+const EnumEntry<uint16_t> PEHeaderMagic[] = {
+    {uint16_t(COFF::PE32Header::PE32), "PE32"},
----------------
alexander-shaposhnikov wrote:
> static constexpr  ?
`constexpr` suffices.

A non-template variable of non-volatile const-qualified type having namespace-scope has internal linkage ([basic.link]), so no need for `static`.


================
Comment at: llvm/tools/llvm-objdump/COFFDump.cpp:166
+
+  static const char *DirName[16] = {
+      "Export Directory [.edata (or where ever we found it)]",
----------------
alexander-shaposhnikov wrote:
> constexpr ?
constexpr applies to the object declaration (`DirName` is a constexpr) which has a different meaning: the element type will be `char*` which will lead to `-Wwrite-strings`.

`static constexpr const char *DirName[16]` works but it is just unnecessarily verbose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113356