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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
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>
----------------
jhenderson wrote:
> 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.
I've experimented with de-templating and it looks not so bad as I thought it seems.
I've posted the D84323 as a possible follow-up that we can land on top if we decide to detemplate it.


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

https://reviews.llvm.org/D83935