[PATCH] D83935: [llvm-readobj] - Move out the common code from printRelocations() methods.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, I think, but it would be wise to get @MaskRay's thoughts too.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3636-3647
+void GNUStyle<ELFT>::printRelReloc(const ELFO *Obj, unsigned SecIndex,
+                                   const Elf_Shdr *SymTab, const Elf_Rel &R,
+                                   unsigned RelIndex) {
+  printRelRelaReloc(Obj, SecIndex, SymTab, R, RelIndex);
+}
+
+template <class ELFT>
----------------
grimar wrote:
> jhenderson wrote:
> > It seems to me like this pair of functions (and the equivalent LLVM pair) is unnecessary. You should be able to call `printRelRelaReloc` directly.
> `printRelRelaReloc` is a non-virtual template method, defined for both `LLVMStyle<ELFT>` and `GNUStyle<ELFT>` output styles.
> 
> ```
> template <class ELFT>
> template <class RelTy>
> void LLVMStyle<ELFT>::printRelRelaReloc(const ELFO *Obj, unsigned SecIndex,
>                                         const RelTy &Rel, unsigned RelIndex,
>                                         const Elf_Shdr *SymTab)
> ```
> 
> ```
> template <class ELFT>
> template <class RelTy>
> void GNUStyle<ELFT>::printRelRelaReloc(const ELFO *Obj, unsigned SecIndex,
>                                        const Elf_Shdr *SymTab, const RelTy &R,
>                                        unsigned RelIndex)
> ```
> 
> The only way I can imagine how to call it directly from the base class is to detemplate it, make it virtual and return to having `Elf_Rela R, bool IsRela` or something alike in the signature, what didn't work good (D83871).
> Or, may be we can pass both `Elf_Rela` and `Elf_Rel` at the same time. E.g:
> 
> ```
> void GNUStyle<ELFT>::printRelRelaReloc(const ELFO *Obj, unsigned SecIndex,
>                                        const Elf_Shdr *SymTab, unsigned RelIndex, const Elf_Rela *Rela, const Elf_Rela *Rel) {
> assert((Rela || Rel) && !(Rela && Rel));
> ...
> }
> ```
> 
> or introduce a helper struct to store either both relocations ot `Elf_Rel` + optional addend. In all that cases this needs `printRelRelaReloc` to be detemplated and virtualized. Want me to try this way?
No, it's okay. I hadn't quite registered templates + virtual methods not combining well in this way.


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

https://reviews.llvm.org/D83935