[PATCH] D63843: [Object][XCOFF] Add support for 64-bit file header and section header dumping.

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:162
+
+  // Constuctor and create function. Constructor builds only a base class
+  // object, while the 'create' function fills-out the XCOFF specific info
----------------
Typo: "Constuctor [//sic//]".

Suggestion:
Constructor and "create" factory function. The constructor is only a thin wrapper around the base constructor. The "create" function fills out the XCOFF-specific information and performs the error checking along the way.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:168
+                                                           MemoryBufferRef MBR);
+  // Helper for parsing the StringTable, returns an 'Error' if parsing failed
+  // and an XCOFFStringTable if parsing succeeded.
----------------
I would suggest adding an empty line before this comment block. Also, break the sentence just before "returns" (where the comma is).


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:173
+
+  // Make a freind so it can call the private 'create' function.
+  friend Expected<std::unique_ptr<ObjectFile>>
----------------
s/freind/friend/;


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:223
 
-  XCOFFObjectFile(MemoryBufferRef Object, std::error_code &EC);
 
+  bool is64Bit() const;
----------------
If the extra empty line is meant to signify that the interface functions below are not inherited from a base class, then perhaps a comment would work better.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:227
   const XCOFFSymbolEntry *getPointerToSymbolTable() const {
     return SymbolTblPtr;
   }
----------------
This is a public function, so there should be an assertion regarding use when `is64Bit()` is `true`.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:248
+  int32_t getRawNumberOfSymbolTableEntries32() const;
+  // The sanitized value appropriate to use an index into the symbol table.
+  uint32_t getLogicalNumberOfSymbolTableEntries32() const;
----------------
Add an empty line before this. Also, missing "as" before "an index".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63843