[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:78-80
+  PlainPrinterBase(raw_ostream &OS, raw_ostream &ES, bool PrintAddress = false,
+                   bool PrintFunctionNames = true, bool PrintPretty = false,
+                   int PrintSourceContext = 0, bool Verbose = false)
----------------
aorlov wrote:
> 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.
> 
You don't have to have a constructor for the PrinterConfig class. Instead, you do something like the following:
```
class PrinterConfig {
public:
  bool PrintAddress = false; // Default initializer is optional.
  bool PrintFunctions = false;
  ...
};

void setupConfig(...)
{
  PrinterConfig Config;
  Config.PrintAddress = Args.hasArg(OPT_addresses);
  Config.PrintFunctions = Opts.PrintFunctions != FunctionNameKind::None;
  Config.ES = errs();
  ...
}
```
The difference is that you no longer have a list of boolean arguments that can easily end up in the wrong order. It also avoids potential for things like this:
```
LLVMPrinter(outs(), errs(), true, false, true, 0, false);
```
which I hope you'll agree is not all that legible.

This is similar to how the LLD config is created - some options are default initialised, and then argument parsing populates the remainder.

As this is about making the Printer interface nicer, it is part of this patch.

It's also worth pointing out that this removes the duplication of the big argument list where you construct the LLVM/GNUPrinter instances in the llvm-symbolizer code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98994