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

[Jason Liu via Phabricator via llvm-commits]

jasonliu marked 2 inline comments as done.
jasonliu added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:252
+  static constexpr uint64_t InvalidRelocOffset =
+      std::numeric_limits<uint64_t>::max();
+
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > MaskRay wrote:
> > > hubert.reinterpretcast wrote:
> > > > `0xffffffff'ffffffffULL`
> > > Will UINT64_C(-1) be clearer?
> > You mean `-UINT64_C(1)`? I don't think so. My experience is that people are not familiar with these macros.
> I would use std::numeric_limits<uint64_t>::max(), since it's clearer to me than a literal. It's meaning seems obvious.
This was my original implementation, I don't have anything against it.

@hubert.reinterpretcast Any comments?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:18-20
+enum { RELOC_OVERFLOW = 65535 };
+enum : uint8_t { FUNCTION_SYM = 0x20, SYM_TYPE_MASK = 0x07 };
+enum : uint16_t { NO_REL_MASK = 0x0001 };
----------------
jhenderson wrote:
> At this point would it just make more sense to make these static constant literals?
> 
> ```
> // TODO: Use appropriate naming convention - are these spec-defined, or just local names?
> static const int RELOC_OVERFLOW = 65535; // Use an appropriate type here.
> static const uint8_t FUNCTION_SYM = 0x20;
> static const uint8_t SYM_TYPE_MASK = 0x07;
> static const uint16_t NO_REL_MASK = 0x0001;
> ```
> 
This was a drive-by fix because I touched this line when I want to put in InvalidRelocOffset. Now, it's irrelevant to the current patch, and I will put it to its original state. 


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

https://reviews.llvm.org/D75131