[PATCH] D104646: [AIX][XCOFF] llvm-readobj 64-bit relocation reading support

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

For a patch advertised as llvm-readobj relocation dumping support, I wouldn't expect to see test changes in the CodeGen directory.

If you need the llvm-readobj changes to test some other functionality in other locations, you should make the llvm-readobj patch first, and then add testing for the other part in a separate patch.

I've said this elsewhere on XCOFF patches, but I really don't like the sort of duplication that the 32-bit/64-bit implementations in the ObjectFile code have. It seems to me like you should be able to avoid it by some additional abstraction. Take a look at the equivalent ELF code - there we use templates to avoid needing to duplicate most code, and/or common objects that hide the details from the user.



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:136
       continue;
-    auto Relocations = unwrapOrError(Obj.getFileName(), Obj.relocations(Sec));
     if (Relocations.empty())
----------------
These changes to use warnings are good, but should be their own separate patch, along with appropriate testing in tools/llvm-readobj.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:141
+
+    const auto Relocations = ErrOrRelocations.get();
     if (Relocations.empty())
----------------
It's not clear to me what the type of `Relocations` is here, and similarly the `ErrOrRelocations` above. Don't use `auto` unless the type is obvious from the RHS (or for some iterator cases). See the Coding Standards for details.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:147-148
                   << " {\n";
-    for (auto Reloc : Relocations) {
-      StringRef SymbolName = unwrapOrError(
-          Obj.getFileName(), Obj.getSymbolNameByIndex(Reloc.SymbolIndex));
+    for (const auto &Reloc : Relocations) {
+      auto ErrOrSymbolName = Obj.getSymbolNameByIndex(Reloc.SymbolIndex);
+      if (Error E = ErrOrSymbolName.takeError())
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104646