[PATCH] D122887: [llvm-pdbutil] Move global state (Filters) inside LinePrinter class.
Alexandre Ganea via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 1 08:53:03 PDT 2022
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
More information about the llvm-commits
mailing list