[PATCH] D75131: [XCOFF][AIX] Enable -r option for llvm-objdump

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:324
+  if (is64Bit())
+    report_fatal_error("64-bit support not implemented yet.");
+  const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
----------------
jhenderson wrote:
> Here and all report_fatal_error calls: remove the trailing full stop.
We have been trying to full stops for XCOFF work for invocations of report_fatal_error, llvm_unreachable, and assert in order to be consistent within our patches and after having observed that such uses are not uncommon.

For example:
```
llvm/lib/Object/COFFObjectFile.cpp:    report_fatal_error("Section was outside of section table.");
llvm/lib/Object/COFFObjectFile.cpp:      report_fatal_error("Aux Symbol data was outside of symbol table.");
llvm/lib/Object/MachOObjectFile.cpp:    report_fatal_error("Malformed MachO file.");
llvm/lib/Object/MachOObjectFile.cpp:    report_fatal_error("Requested symbol index is out of range.");
```


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:325
+    report_fatal_error("64-bit support not implemented yet.");
+  const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
+  auto RelocationsOrErr = relocations(*SectionEntPtr);
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > Please replace `Typename*` binding with `Declarator *binding`.
> Isn't this handled by clang-format?
It does for me now. I'm not certain it always did so in the past.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:24-25
+  if (SymI == Obj->symbol_end())
+    return make_error<GenericBinaryError>("Could not get relocation symbol.",
+                                          object_error::parse_failed);
+
----------------
jhenderson wrote:
> Errors usually have lower case first letter and no trailing full stop.
Thanks for pointing this out. We will correct this.


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

https://reviews.llvm.org/D75131