[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:59
+                          const ErrorInfoBase &ErrorInfo,
+                          const StringRef ErrorBanner = "") = 0;
+};
----------------



================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:78-80
+  PlainPrinterBase(raw_ostream &OS, raw_ostream &ES, bool PrintAddress = false,
+                   bool PrintFunctionNames = true, bool PrintPretty = false,
+                   int PrintSourceContext = 0, bool Verbose = false)
----------------
I think it would be a good idea to create a helper `PrinterConfig` class or similar, which contains the various Print* and Verbose argument. It will make the interface cleaner and easier to extend in the future.


================
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){};
----------------
jhenderson wrote:
> `StringRef` is designed to be passed by value, as it's already a reference, so is cheap to copy.
Similarly, no need for the `const` anymore. Please delete.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:47
+public:
+  DIPrinter(raw_ostream &OS, raw_ostream &ES) : OS(OS), ES(ES) {}
+  virtual ~DIPrinter(){};
----------------
aorlov wrote:
> 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.
> 
Having seen the impact, I regret asking you to do this. If you prefer to use `OS` and `ES`, it's fine to change back.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:63
+protected:
+  bool PrintAddress;
   bool PrintFunctionNames;
----------------
aorlov wrote:
> 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.
My thinking is that it is actually a beneficial change, even without the other refactoring. It would be good to minimise noise as much as possible.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:34
 // Prints source code around in the FileName the Line.
-void DIPrinter::printContext(const std::string &FileName, int64_t Line) {
+void PlainPrinterBase::printContext(const StringRef FileName, int64_t Line) {
   if (PrintSourceContext <= 0)
----------------



================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:93
 
-DIPrinter &DIPrinter::operator<<(const DILineInfo &Info) {
+void GNUPrinter::print(const DILineInfo &Info, bool Inlined) {
+  if (PrintFunctionNames) {
----------------
If I'm reading this function correctly, I think you can push most of it into the base class, because it's very similar to the LLVM style. If you add a few new functions, something like `printFunctionName`, `printSimpleLocation` (which would take a filename at least), and `printVerbose`, with the `printSimplyLocation` implemented by the subclasses and the base class implementing the other two. The base class `print` would then call these functions in sequence.


================
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)));
----------------
aorlov wrote:
> 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.
>  Weirdness here is because of the bug-to-bug backward compatibility.

One person's bug is another person's feature. For example, you might want to print some custom delimiters between symbolized addresses.

In the existing printers, the invalid input is not considered an error, whereas it might be for e.g. JSON. I therefore think you should have two functions: `printInvalidCommand` and `printError`. For the JSON implementation, `printInvalidCommand` would just forward straight to `printError`, saying something like "invalid input addresses are not accepted with JSON output style" (or possibly stuffing it in a JSON object with some unique tag or something. For the LLVM and GNU implementations, the function would just print and then return without needing to do anything special. It helps simplify the `printError` logic, which I find to be somewhat complicated for an error handling function, if I'm honest.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98994