[PATCH] D67008: [llvm-readobj][XCOFF]implement parsing relocation information for 32-bit xcoff objectfile

Fri Oct 11 10:05:41 PDT 2019

DiggerLin marked an inline comment as done.
DiggerLin added inline comments.

================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:168
+
+bool isRelocationSigned(XCOFFRelocation32 &Reloc);
+
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > sfertile wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > DiggerLin wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > Do these need to be declared in the header? Are they called only in one `.cpp` file? If so, they can be made `static` in the `.cpp` file. Otherwise, it seems odd that these aren't `const` member functions of `XCOFFRelocation32`.
> > > > > > the llvm-readobj is using those function and obj2yaml will use them too.
> > > > > It is still odd to me that these aren't `const` non-static member functions of `XCOFFRelocation32`.
> > > > I think were these originally templated to work with both 32-bit and 64-bit relocations, which explains why they aren't member functions.
> > > Would using CRTP with a base class template work for that case?
> > as Sean's comment, for we only implement 32 bits relocation, we do not use any template for the relocation implement this moment. 
> All the more reason why these should be non-static member functions in the context of this patch.
changed to member functions

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008