[PATCH] D61532: implement of the parsing symbol table for xcoffobjfile and output as yaml format

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:122
   void moveSectionNext(DataRefImpl &Sec) const override;
-  Expected<StringRef> getSectionName(DataRefImpl Sec) const override;
+  std::error_code getSectionName(DataRefImpl Sec,
+                                 StringRef &Res) const override;
----------------
You need to rebase. The signature is `Expected<StringRef> getSectionName(DataRefImpl Sec) const override;` now. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:404
   // Current location within the file.
   uint64_t CurPtr = 0;
 
----------------
The heavylifting parsing work should be done in `createXCOFFObjectFile`. The constructor should be private and be a very thin wrapper.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:438
+  StringTable.Size =
+      *reinterpret_cast<const support::ubig32_t *>(base() + CurPtr);
+
----------------
Use `support::endian::read32be` in `llvm/Support/Endian.h`


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:22
   void dumpHeader();
+  std::error_code dumpSymbols();
 
----------------
For new code, prefer `Error` `Expected<T>` to `std::error_code`. The former enforces error checking and provides better error messages. Avoid `errorToErrorCode` if possible, it is very common to lost messages after converting to an `Error`.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:69
+    if (!SectionNameRefOrErr) {
+      return errorToErrorCode(SectionNameRefOrErr.takeError());
+    }
----------------
redundant braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61532