[PATCH] D134883: [XCOFF] llvm-readobj support decoding the loader section header field for XCOFF object file.

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked an inline comment as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:198
 
-struct LoaderSectionHeader32 {
+template <typename T> struct LoaderSectionHeader {
+  uint32_t getVersion() const { return static_cast<const T *>(this)->Version; }
----------------
jhenderson wrote:
> Reading the code, it seems like the CRTP (and therefore this base class) are unnecessary. In your lambda where you print the fields, you can use the common subclass memeber fields directly, because you've provided the input parameter type of the lamdba as `auto`.
thanks, good idea.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:514-515
+    if (opts::XCOFFLoaderSectionHeader) {
+      Dumper->setPrintLoaderSectionHeader();
+      Dumper->printLoaderSection();
+    }
----------------
jhenderson wrote:
> This is a very odd way of getting it to print a specific part of the loader section. It would be much more normal to pass in booleans into `printLoaderSection`, rather than modifying the state of the dumper to ensure it does print it. Take a look at the `printSymbols` call for example.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134883