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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:31
 
+struct DIRequest {
+  StringRef ModuleName;
----------------
I'd be inclined to omit the `DI` part of this name. We're in a distinct namespace, and I'm not sure the `DI` prefix gives us anything.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:34
+  uint64_t Address = 0;
+  DIRequest(const StringRef &ModuleName, uint64_t Address)
+      : ModuleName(ModuleName), Address(Address){};
----------------
`StringRef` is designed to be passed by value, as it's already a reference, so is cheap to copy.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:47
+public:
+  DIPrinter(raw_ostream &OS, raw_ostream &ES) : OS(OS), ES(ES) {}
+  virtual ~DIPrinter(){};
----------------
Maybe `OutputStream` and `ErrorStream`? `ES` in particular is not a common abbreviation, so it's hard to follow the naming.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:58
+                          const ErrorInfoBase &ErrorInfo,
+                          const StringRef &ErrorBanner = "") = 0;
+};
----------------
See above re. `StringRef`


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:61
+
+class DIPrinterGeneric : public DIPrinter {
+protected:
----------------
I can see why you have this class, but I'm not a fan of the name. Why is this "generic", but the base class isn't, for example? Also, see above re. the `DI` prefix.

How about `PlainPrinterBase` or something like that? The subclasses could be `LLVMPrinter` and `GNUPrinter`.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:63
+protected:
+  bool PrintAddress;
   bool PrintFunctionNames;
----------------
Perhaps we could reduce the number of changes needed in this main part of the refactoring by moving the address printing in a separate prerequisite patch? Does that make sense?


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:70
+  virtual void print(const DILineInfo &Info, bool Inlined) = 0;
+  virtual void printHeader(uint64_t Address);
+  virtual void printFooter() {}
----------------
Why is this `virtual` and `protected`?


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:81
+        PrintSourceContext(PrintSourceContext), Verbose(Verbose) {}
+  virtual ~DIPrinterGeneric() {}
+
----------------
I believe you don't need this, since it's in the base class.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:94-95
+private:
+  void print(const DILineInfo &Info, bool Inlined);
+  void printFooter();
+
----------------
Here and everywhere else, use `override` wherever you are overriding a base class method.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:215-216
+
+  if (ErrorInfo.convertToErrorCode().value() ==
+      int(std::errc::invalid_argument)) {
+    OS << ErrorInfo.message() << '\n';
----------------
This looks extremely brittle. Using `invalid_argument` as an error code is not unusual, and probably doesn't always correspond to the case you think it does.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:78
+template <typename T>
+static void print(const StringRef &ModuleName, uint64_t Address,
+                  Expected<T> &ResOrErr, DIPrinter &Printer) {
----------------
Why have you changed what this function does? What was wrong with the previous approach, where the main printing was handled by the calling code? I'd think you could just pass in the Printer, and then call `Printer.printError` in place of the `logAllUnhandledErrors` function.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:94-96
+  // Print an empty struct in case of Cmd != Command::Frame.
+  if (PrintEmpty && !std::is_same<T, std::vector<DILocal>>::value)
+    Printer.print(DIRequest(ModuleName, Address), T());
----------------
This kind of special-casing shows that this isn't the best design for this code.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:160-163
+    Printer.printError(
+        DIRequest(ModuleName, Offset),
+        StringError(InputString,
+                    std::make_error_code(std::errc::invalid_argument)));
----------------
This probably wants to be a completely different function, since an unparseable command is not really an error. How about another function `printInvalidCommand`? That would help resolve the brittleness I mention above, too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98994