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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:419
+      is64Bit() ? getNumberOfSymbolTableEntries64()
+                : getLogicalNumberOfSymbolTableEntries32();
+  SymDRI.p =
----------------
jhenderson wrote:
> jasonliu wrote:
> > 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. 
> From my experience working with tools that had to support 32-bit and 64-bit ELF, you don't worry about the underlying type in most cases and always use the larger type. The same probably applies here. Of course, it becomes a bit moot if you add a common getter interface as suggested out-of-line, because those getters will have to return the larger of the two return types anyway.
> 
> Is there a strong reason to not use the larger type everywhere?
> Is there a strong reason to not use the larger type everywhere?

I don't know what strength this reason has, but we had noticed that some of the tools do not reflect the width of the 32-bit format fields very well (even for relatively uninterpreted output). Where the producer of the binary is under development, developers are better served if the tools emit the correct width for fields in the format.


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

https://reviews.llvm.org/D85774