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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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; }
----------------
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`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:166
+void XCOFFDumper::printLoaderSectionHeader(uintptr_t LoaderSectionAddr) {
+
+  DictScope DS(W, "Loader Section Header");
----------------
Nit: don't start functions with a blank line.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:169
+
+  auto printLoadSecHeaderCommon = [&](auto *LDHeader) {
+    W.printNumber("Version", LDHeader->getVersion());
----------------
Nit: Lambdas are function objects. The consensus is that their names should be written like variables, because they are passed around like them (i.e. `PrintLoadSecHeaderCommon`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134883