[PATCH] D118633: [Symbolizer] Add Build ID flag to llvm-symbolizer.

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/Symbolize.h:61-66
   Expected<DILineInfo> symbolizeCode(const ObjectFile &Obj,
                                      object::SectionedAddress ModuleOffset);
   Expected<DILineInfo> symbolizeCode(const std::string &ModuleName,
                                      object::SectionedAddress ModuleOffset);
+  Expected<DILineInfo> symbolizeCode(ArrayRef<uint8_t> BuildID,
+                                     object::SectionedAddress ModuleOffset);
----------------
jhenderson wrote:
> Not necessarily something that you need to do right now, but nonetheless worth thinking about: it's clear by adding a third variation of each of these methods that some sort of refactoring would be appropriate to simplify extensibility of this code. In this case, I'd probably make each of the different input types (object, module name, build ID) a separate class, with the relevant methods attached. `symbolizeCode` would then just become something like:
> ```
> template <typename T>
> Expected<DILineInfo> symbolizeCode(const T &Input, object::SectionedAddress ModuleOffset) {
>   return Input.symbolizeCode(ModuleOffset, ...);
> }
> ```
> and so on. Adding a new input kind wouldn't then require modifying this class again, keeping this class stable, and just adding new classes for each input kind.
Yeah, this occurred to me too. I'll take a stab at this afterwards and see if I can design a clean abstraction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118633