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

Jason Liu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 9 13:04:56 PDT 2020


jasonliu added a comment.

In D85774#2217649 <https://reviews.llvm.org/D85774#2217649>, @jhenderson wrote:

> It seems to me like this should be using inheritance here. You have a base class that has the common members, and provides pure virtual declarations of the various getters, with the sub-classes defining them to do the right thing. Yes, it would introduce a number of getters, but I feel like it would make everything a bit cleaner from a usability standpoint. In most cases, you then don't need any `is64Bit` queries, because the getters hide that from you.

@jhenderson I tried to use inheritance as suggested. But inheritance would mean I need to use pointers to enable the runtime polymorphism. Then there is a life time issue that need to be managed when using pointers. The easier way to achieve that is to return via unique_ptr. But using unique_ptr introduced usability issue in the caller/user side, as we would see std::move, SymbolRef.get() before getting to the query we want. Also underneath of the unique_ptr, new/delete is not very efficient as well. 
In the end, I tried to solve this in similar manner as COFF does, which is using a view class without inheritance. Although the downside of it is we basically have an if query in every call to the view class to differentiate which version (32/64) we are having right now, the good thing is that caller side is much more cleaner.


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

https://reviews.llvm.org/D85774



More information about the llvm-commits mailing list