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

[Alex Orlov via Phabricator via llvm-commits]

aorlov marked 10 inline comments as done.
aorlov added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:47
+public:
+  DIPrinter(raw_ostream &OS, raw_ostream &ES) : OS(OS), ES(ES) {}
+  virtual ~DIPrinter(){};
----------------
jhenderson wrote:
> Maybe `OutputStream` and `ErrorStream`? `ES` in particular is not a common abbreviation, so it's hard to follow the naming.
I have changed the OS and ES to the requested names. On my taste the code became less readable, as the lines are longer and more line breaks are involved. Personally I’d prefer shorter names, OS is already used name people got used to, and ES is in line.



================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:63
+protected:
+  bool PrintAddress;
   bool PrintFunctionNames;
----------------
jhenderson wrote:
> 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?
I can propose that small patch. Though it does not seem worth it as it will save just a few lines in this one, wouldn’t make a big difference.


================
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) {
----------------
jhenderson wrote:
> 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.
I didn’t change the error() function. 

I have implemented a new function which effectively does print-res-or-err and implements the logic from main which used to be a sequence of branches specific to different data types. Since the handling of different data types are similar, we could use the template machinery to highlight the similarity. This produces a smaller cleaner code.

Printing out errors is a trivial one-liner, and does not deserve a separate function. That’s a small part of this new print-res-or-err function.

I have named this print-res-or-err function “print”, since I think it is a better name. print(ResOrErr) looks clean and right to the point. Much better than printResOrErr(ResOrErr).



================
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());
----------------
jhenderson wrote:
> This kind of special-casing shows that this isn't the best design for this code.
You are right. We do not need this special case here.


================
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)));
----------------
jhenderson wrote:
> 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.
> since an unparseable command is not really an error

What is so special with the unparseable command besides someone attached some questionable logic to that case in the current implementation? This is one of regular errors, which shall be reported with regards to the specified output style.
It might look a bit different just because it is detected here and not returned by a library code. Does not make it something other than error, though.
Weirdness here is because of the bug-to-bug backward compatibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98994