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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast marked 2 inline comments as done.
hubert.reinterpretcast added a comment.

LGTM to land as-is. Not sure if other people have an opinion about the `const`.
@DiggerLin, I believe you have had a number of patches committed into the project. I think you can request commit access and land this yourself. Thanks.



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:58
+  static constexpr unsigned SectionFlagsTypeMask = 0xffffu;
   const XCOFFObjectFile &Obj;
 };
----------------
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?


================
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
----------------
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.


================
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;
----------------
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`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68575