[PATCH] D68575: [llvm-readobj][xcoff] implement parsing overflow section header.

[Digger via Phabricator via llvm-commits]

DiggerLin marked 3 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:58
+  static constexpr unsigned SectionFlagsTypeMask = 0xffffu;
   const XCOFFObjectFile &Obj;
 };
----------------
hubert.reinterpretcast wrote:
> Add a blank line here. Also, I am wondering if this should be part of `llvm/BinaryFormat/XCOFF.h` (perhaps in `SectionHeader32`, or in a base class thereof when 64-bit support lands).
for consistent with  SectionFlagsReservedMask, puting define SectionFlagsTypeMask here too, I think we maybe need to create a NFC patch to put SectionFlagsReservedMask  and SectionFlagsTypeMask  in the xcoff.h


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:455
+    case XCOFF::STYP_TYPCHK:
+      // TODO : The interpretation of loader, exception, type check section
+      // headers are different from that of generic section header. We will
----------------
hubert.reinterpretcast wrote:
> The "TODO" still has a colon surrounded by spaces on both sides after it. I do not think that we have been using colons after "TODO".
> 
> Still missing "and" before "type check section headers".
> 
> Still missing "s" after "generic section header".
> 
> Typo "seciton" is still present.
changed as suggestion


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:463
+    }
+    // For now we just dump the section type flags.
+    if (SectionType & SectionFlagsReservedMask)
----------------
hubert.reinterpretcast wrote:
> Suggestion: "For now we just dump the section type portion of the flags."
changed as suggestion.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68575