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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
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);
----------------
aganea wrote:
> There's no need to pass the `Printer` here, you already have a reference in `HeaderScope`.
Very good point.


================
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 =
----------------
aganea wrote:
> Could you rather pass `Printer.Filters` as an argument rather?
Now I am passing the `Filters`. I have to add extra logic because `Filters` should be `optional<..>`.


================
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; }
----------------
aganea wrote:
> 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`.
The function has been moved to its original location.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/LinePrinter.h:104
   bool UseColor;
+  FilterOptions Filters;
 
----------------
aganea wrote:
> 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).
Now is `const reference`.


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

https://reviews.llvm.org/D122887