[PATCH] D98994: NFC. Refactored DIPrinter for better support of new print styles.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:34-35
+  uint64_t Address = 0;
+  Request(StringRef ModuleName, uint64_t Address)
+      : ModuleName(ModuleName), Address(Address){};
+};
----------------
I probably wouldn't bother with a ctor here - uses can use braced init to construct it with a similar amount of syntax.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:333-355
+  if (OutputStyle == DIPrinter::OutputStyle::GNU)
+    Printer.reset(new GNUPrinter(outs(), errs(), Config));
+  else
+    Printer.reset(new LLVMPrinter(outs(), errs(), Config));
 
   std::vector<std::string> InputAddresses = Args.getAllArgValues(OPT_INPUT);
   if (InputAddresses.empty()) {
----------------
If the DIPrinter represents whether to use GNU or LLVM style printing - I don't think the output style should otherwise be exposed to the code (if it is, it seems to suggest that DIPrinter doesn't sufficiently encapsulate the concept of output styling)

Then that DIPrinter::OutputStyle enum could move into llvm-symbolizer.cpp since it won't need to be known by any other code.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:334-336
+    Printer.reset(new GNUPrinter(outs(), errs(), Config));
+  else
+    Printer.reset(new LLVMPrinter(outs(), errs(), Config));
----------------
Probably use std::make_unique here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98994