[PATCH] D104613: [llvm-readobj][XCOFF] Add support for printing the String Table.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:18
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/BinaryFormat/XCOFF.h"
----------------
I don't think this has been clang-formatted. Include lists should be in alphabetical order.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:190
 
+Expected<DenseMap<uint64_t, StringRef>>
+XCOFFObjectFile::getStringTable() const {
----------------
Higuoxing wrote:
> Since we are modifying `lib/Object/XCOFFObjectFile.cpp` and this function may return an error, I think we need to write unittest for this function. What do you think, @jhenderson ?
A gtest unit test is always preferable for libraries, in my opinion, but I don't know if the XCOFF stuff has extensive unit testing, and where there's a tool that handles this error, it's probably sufficient to test it in the tool lit tests.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:464
+  if (!StringTableMap)
+    reportWarning(StringTableMap.takeError(), Obj.getFileName());
+  for (const auto &EntryPair : StringTableMap.get()) {
----------------
Always use `reportUniqueWarning` in preference to `reportWarning`. The current `reportWarning` implementation  may mean the same warning is reported repeatedly, and we are trying to migrate to `reportUniqueWarning` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104613