[PATCH] D135887: [XCOFF] llvvm-readobj support display symbol table of loader section of xcoff object file.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Looks good, but probably worth @Esme taking a quick look to confirm that they're happy too.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:93
+template <typename T>
+Expected<StringRef> getLoadSectionSymName(const T *LDHeader, uint64_t Offset) {
+  if (LDHeader->LengthOfStrTbl > Offset)
----------------
DiggerLin wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > 
> > Marked as done, but this comment hasn't been addressed. Please don't mark comments as done if you haven't done them.
> I used the abbr Name in function name , otherwise  the function name is too long. For example:
> Sec for Section.
> StrTbl for StringTable.
> Load for Loader.
> 
> 
> 
I'm okay with some abbreviations (Sec, Sym and StrTbl are all fine), but "Load" -> "Loader" is not an obvious one to me, hence my objection.

Regarding marking things as done - if you disagree with my comment, that's fine, but please say so and explain why so that we can have a discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135887