[PATCH] D122887: [llvm-pdbutil] Move global state (Filters) inside LinePrinter class.

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

Thanks for following up on this @CarlosAlbertoEnciso!



================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/InputFile.h:179
                           const Optional<PrintScope> &HeaderScope,
-                          CallbackT Callback) {
+                          const LinePrinter &Printer, CallbackT Callback) {
   AutoIndent Indent(HeaderScope);
----------------
There's no need to pass the `Printer` here, you already have a reference in `HeaderScope`.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/InputFile.h:193
   for (const auto &SG : Input.symbol_groups()) {
-    if (shouldDumpSymbolGroup(I, SG))
+    if (Printer.shouldDumpSymbolGroup(I, SG))
       if (auto Err =
----------------
Could you rather pass `Printer.Filters` as an argument rather?


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/InputFile.h:207
     InputFile &File, const Optional<PrintScope> &HeaderScope,
+    const LinePrinter &Printer,
     llvm::function_ref<Error(uint32_t, const SymbolGroup &, SubsectionT &)>
----------------
Same here and below, `HeaderScope.P` could be used instead of passing it by arg.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/LinePrinter.h:89
 
+  bool shouldDumpSymbolGroup(uint32_t Idx, const SymbolGroup &Group) const;
+  uint32_t getDumpModi() const { return Filters.DumpModi; }
----------------
I think you can leave this function where it was initially, and instead just pass `Filters` as an argument at the call site. It doesn't need to know about the `LinePrinter` since they only need the `Filters`.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/LinePrinter.h:104
   bool UseColor;
+  FilterOptions Filters;
 
----------------
On a second thought, I think it'd better if this was a (const?) reference (`FilterOptions &Filters`), to make it explicit that the data is held somewhere else (by the caller).


================
Comment at: llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp:484
   return iterateSymbolGroups(
-      File, PrintScope{P, 11},
+      File, PrintScope{P, 11}, P,
       [&](uint32_t Modi, const SymbolGroup &Strings) -> Error {
----------------
You shouldn't be needing those changes from here below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122887