[PATCH] D75131: [XCOFF][AIX] Enable -r option for llvm-objdump
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 17 08:31:23 PDT 2020
hubert.reinterpretcast added inline comments.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:337
+ report_fatal_error("64-bit support not implemented yet.");
+ const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
+ auto RelocationsOrErr = relocations(*SectionEntPtr);
----------------
clang-format here too. Please check throughout the patch.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:360
+ // relocation offset relative to the start of the section.
+ if (Sec32->VirtualAddress <= RelocAddress &&
+ (Sec32->VirtualAddress + Sec32->SectionSize) > RelocAddress) {
----------------
I believe it is more idiomatic to place the range boundaries on either end of the comparison:
```
Sec32->VirtualAddress <= RelocAddress && RelocAddress < (Sec32->VirtualAddress + Sec32->SectionSize)
```
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:364
+ }
+ Sec32++;
+ }
----------------
I believe it is customary to use prefix increment when possible: https://llvm.org/docs/CodingStandards.html#prefer-preincrement
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:389
+ report_fatal_error("64-bit support not implemented yet.");
+ const XCOFFRelocation32 *Reloc = viewAs<XCOFFRelocation32>(Rel.p);
+ return Reloc->Type;
----------------
Minor nit: Functions that can be "simple one-liners" should be written as such.
The 64-bit part aside, we can have just the return statement.
The overhead of the declaration for `Reloc` is a large proportion of this function, especially considering its (lack of) utility here.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:1
+//===-- XCOFFDump.cpp - XCOFF-specific dumper ---------------------*- C++ -*-===//
+//
----------------
Line exceeds 80 columns.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75131/new/
https://reviews.llvm.org/D75131
More information about the llvm-commits
mailing list