[PATCH] D85774: [XCOFF][AIX] Enable tooling support for 64 bit symbol table parsing

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:204
+
+  return toSymbolEntry(Symb)->Obj32.Value;
 }
----------------
DiggerLin wrote:
> if we define 
> getValue() for the  XCOFFSymbolRef
> we rewrite the code as
> Expected<uint64_t> XCOFFObjectFile::getSymbolAddress(DataRefImpl Symb) const {
> return XCOFFSymRef(Symb, this).getValue();
> }
Please see my other comments regarding combining the 32bit and 64 bit version into 1 function.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:211
+
+  return toSymbolEntry(Symb)->Obj32.Value;
 }
----------------
DiggerLin wrote:
> same as above comment.
Please see my other comments regarding combining the 32bit and 64 bit version into 1 function.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:419
+      is64Bit() ? getNumberOfSymbolTableEntries64()
+                : getLogicalNumberOfSymbolTableEntries32();
+  SymDRI.p =
----------------
DiggerLin wrote:
> can we add new member function as getNumberOfSymbolTableEntries()
> {
>   return is64Bit() is64Bit() ? getNumberOfSymbolTableEntries64()
>                 : getLogicalNumberOfSymbolTableEntries32();
> }
> 
> the function can also use in 
> XCOFFObjectFile::create()
> and
> getSymbolNameByIndex()
About all the comments mentioning if we could combining the 32bit and 64 bit version into 1 function.
I don't think it's good idea because people would ignore the fact that they are returning different types underneath. 


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

https://reviews.llvm.org/D85774