[PATCH] D56842: [llvm-objdump] - Move getRelocationValueString and dependenices out of the llvm-objdump.cpp

[George Rimar via Phabricator via llvm-commits]

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


================
Comment at: tools/llvm-objdump/llvm-objdump.h:139
+                              const object::RelocationRef &RelRef,
+                              llvm::SmallVectorImpl<char> &Result);
+
----------------
sbc100 wrote:
> Why not continue to use overloads?
There are few benefits from not using them here, and I see no profit.

Now we have a code that looks like:


```
static std::error_code getRelocationValueString(const ELFObjectFileBase *Obj,
                                                const RelocationRef &Rel,
                                                SmallVectorImpl<char> &Result) {
  if (auto *ELF32LE = dyn_cast<ELF32LEObjectFile>(Obj))
    return getRelocationValueString(ELF32LE, Rel, Result);
  if (auto *ELF64LE = dyn_cast<ELF64LEObjectFile>(Obj))
    return getRelocationValueString(ELF64LE, Rel, Result);
  if (auto *ELF32BE = dyn_cast<ELF32BEObjectFile>(Obj))
    return getRelocationValueString(ELF32BE, Rel, Result);
  auto *ELF64BE = cast<ELF64BEObjectFile>(Obj);
  return getRelocationValueString(ELF64BE, Rel, Result);
}

static std::error_code getRelocationValueString(const RelocationRef &Rel,
                                                SmallVectorImpl<char> &Result) {
  const ObjectFile *Obj = Rel.getObject();
  if (auto *ELF = dyn_cast<ELFObjectFileBase>(Obj))
    return getRelocationValueString(ELF, Rel, Result);
  if (auto *COFF = dyn_cast<COFFObjectFile>(Obj))
    return getRelocationValueString(COFF, Rel, Result);
  if (auto *Wasm = dyn_cast<WasmObjectFile>(Obj))
    return getRelocationValueString(Wasm, Rel, Result);
  if (auto *MachO = dyn_cast<MachOObjectFile>(Obj))
    return getRelocationValueString(MachO, Rel, Result);
  llvm_unreachable("unknown object file format");
}
```

It seems to be completely unreadable. Looking at this code I can hardly realize that dispatcher function `getRelocationValueString`
will call ELF version of `getRelocationValueString` which will call one more `getRelocationValueString` basing on `ELFT`.
It is painful to trace.

The second benefit is that other methods in this header contain the COFF/ELF/Wasm/MachO prefixes/suffixes.
I think we might want to regroup them by the target, having them mixed does not look good.
So renaming will be important for consistency.

These implementations have no much common inside, after all, to name them in the same way. Logically this is a 4 really different implementations.


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

https://reviews.llvm.org/D56842