[PATCH] D87087: [llvm-readobj/elf] - Generalize the code for printing dynamic relocations. NFCI.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4463
     for (const Elf_Rela &Rela : this->dumper()->dyn_relas())
-      printDynamicRelocation(Rela);
+      printRelaReloc(Rela, /*RelNdx=*/0, /*Sec=*/nullptr, /*Symtab=*/nullptr);
   }
----------------
jhenderson wrote:
> I'm not a fan of having to go through the `printRelaReloc`, just to get to the `printDynamicRelocation` method which was being called before. I take it this is because `printDynamicRelocation` is templated and can't be `virtual`? I'm beginning to wonder if using the template solution to the Elf_Rel/Elf_Rela problem was a mistake - it's making the code harder to follow, due to there being unnecessary layers of functions (and in this case, parameters). Maybe we should use a class that can represent either an Elf_Rel or Elf_Rela, ideally with an implicit conversion constructor that is used instead of the two of them. That way, the template can be avoided, and the `printDynamicRelocation` could become a part of the interface. To avoid confusion, the class would not be an Elf_Rela (so that we aren't misrepresenting Elf_Rel instances internally). What do you think?
Yes, I think it makes sense. I'll am going to try to introduce such class and experiment with the code a bit.


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

https://reviews.llvm.org/D87087