[PATCH] D155045: [llvm-objdump] Create ObjectFile specific dumpers

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added a comment.

Thanks for the review. There are recently a lot of XCOFF llvm-objdump patches and I unfortunately don't have much time reading them, but a pattern I noticed is that `llvm-objdump.cpp` gets more and more object file format specific functions, which is probably not a good sign.

I'll land this patch soon to hopefully help object file format specific functions reside in *Dump.cpp` files. I think @jhenderson would agree with the direction and am happy to address his post-commit suggestions.



================
Comment at: llvm/tools/llvm-objdump/COFFDump.cpp:40
 
-class COFFDumper {
+class COFFDumper : public Dumper {
 public:
----------------
mtrofin wrote:
> (Comment for discussion, not blocking this patch) should `ObjDumper` be reused by both readobj and objdump - this was prompted by my going to initially suggest - also for a potential future patch - renaming `Dumper` to something more specific (like `ObjDumper`) - then realized the name was taken. Or maybe one could be `ObjDumper` and the other `ReadObjDumper`?
> 
> Anyway, this can go offline on discourse.
Thank you for bring up the topic. I prefer a different name `Dumper` for llvm-objdump because:

* `llvm::ObjDumper` is taken by llvm-readobj.
* We cannot reuse `llvm::ObjDumper` for llvm-objdump, so a separate class is needed.
* `llvm::objdump::ObjDumper` is probably not a good idea, considering that we do `using namespace llvm;`

Therefore, I picked `llvm::objdump::Dumper` for now.

Classes like `ELFDumper` in an unnamed namespace do not suffer ODR violation problems, so name collision is fine.

I agree that renaming `llvm::ObjDumper` to be something like `llvm::ReadObjDumper` or adding to a new namespace makes sense. That change should be a separate refactoring for llvm-readobj.


================
Comment at: llvm/tools/llvm-objdump/ELFDump.cpp:44
+template <class ELFT>
+static std::unique_ptr<Dumper> createDumper(const ELFObjectFile<ELFT> &Obj) {
+  return std::make_unique<ELFDumper<ELFT>>(Obj);
----------------
mtrofin wrote:
> does this buy much - looks like `objdump::createELFDumper` below would just look about the same, and (subjectively) even maybe a bit more clear?
This function is entirely used for the template argument deduction, so that I don't need to do `return createDumper<Dumper<ELF32LE>>(*O);` below, with the risk of making a template argument mistake...



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155045