[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:
> DiggerLin wrote:
> > 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
> Okay, I agree. Would you mind posting such an NFC patch after this patch lands?
OK, I will do it.


================
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:
> DiggerLin wrote:
> > 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
> For future reference, I believe we have been using "Oxford commas". That is, a comma before the "and" before (in this case) the third list item, would be appropriate.
OK, got it , thanks


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:46
   template <typename T> void printSectionHeaders(ArrayRef<T> Sections);
+  template <typename T> void printGenericSectionHeader(T &Sec) const;
+  template <typename T> void printOverflowSectionHeader(T &Sec) const;
----------------
hubert.reinterpretcast wrote:
> I am not sure that I see a meaningful difference between the functions here that are `const` and the ones that are not. Given that there are already precedent cases of printing methods of `ObjDumper` subclasses that are `const`, I am okay with adding new ones that are `const` if we have reason to believe they will remain `const`.
when I create a new function , I  make as many functions const as possible so that accidental changes to objects are avoided.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68575