[PATCH] D59419: [XCOFF] Add functionality for parsing AIX XCOFF object files header .

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:36
+
+struct XCOFFFileHeader {
+  support::ubig16_t Magic;
----------------
rnk wrote:
> sfertile wrote:
> > Rather then defining these in the `llvm::object` namespace why not drop the `XCOFF` part of the name and make the defintion nested inside `XCOFFObjectFile`?
> (Not to bikeshed too much...) Personally, I'd leave this as is. It's consistent with the other object formats. It's useful to be able to do `using namespace llvm::object;`, but you cannot shorten `XCOFFObjectFile::Header` without a typedef or type alias.
I am agreed with Reid Kleckner.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:44
+  
+  support::big32_t SymbolTableOffset; // File offset to symbol table.
+  support::big32_t NumberOfSymTableEntries;
----------------
hubert.reinterpretcast wrote:
> When `_XCOFF_UOFFSET` is set, the AIX headers treat this field as unsigned. Indications are that values greater than those of the corresponding signed type are indeed valid (despite difficulties in using files with such values with the AIX system linker).
changed to support::ubig32_t SymbolTableOffset


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:53
+template <typename T>
+static std::error_code getObject(const T *&Obj, MemoryBufferRef M,
+                                 const void *Ptr,
----------------
sfertile wrote:
> The only use of this is in XCOFFObjectFile.cpp,  it should be defined there instead of in the header.
OK.  I can move it to XCOFFObjectFile.cpp


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:241
+  // Check the whether the binary file contains an XCOFF Header.
+  if (!checkSize(Data, EC, sizeof(XCOFFFileHeader)))
+    return;
----------------
hubert.reinterpretcast wrote:
> `getObject` below does size checking too. What does having this check here add?
I have deleted the checkSize function definition and removed the check size here


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

https://reviews.llvm.org/D59419