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

[Alex Orlov via Phabricator via llvm-commits]

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


================
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)
----------------
jhenderson wrote:
> 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.
Adding PrinterConfig does not seem to make this patch better. It would be the same set of flags but passed in to the PrinterConfig constructor, which in its turn will be passed to a DIPrinter instance a line or two below that.

Here is the code snippet to illustrate:
```
PrinterConfig PrnCfg(Args.hasArg(OPT_addresses),
                     Opts.PrintFunctions != FunctionNameKind::None,
                     Args.hasArg(OPT_pretty_print),
                     SourceContextLines, Args.hasArg(OPT_verbose));

if (OutputStyle == DIPrinter::OutputStyle::GNU)
  Printer.reset(new GNUPrinter(outs(), errs(), PrnCfg));

```
+ PrinterConfig definition noise.

Note JSONPrinter does not require PrinterConfig at all.

I do not oppose the idea of having PrinterConfig, but I do oppose the idea of making it a dependency to this patch. If someone feels strong for having such class, it could be added later out of the scope of this patch.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98994