[PATCH] D126980: [Symbolize] Add log markup --filter to llvm-symbolizer.
Peter Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 23 03:41:12 PDT 2022
peter.smith added a comment.
Only a few small comments. May be worth calling Filter.cpp and Filter.h MarkupFilter.cpp and MarkupFilter.h as there could be non-markup based filters in the future. Not a strong opinion as these filenames could be changed later if needed.
Could be worth adding to the release notes as a new feature for llvm-symbolizer.
================
Comment at: llvm/docs/CommandGuide/llvm-symbolizer.rst:250
+.. option:: --filter
+
----------------
Looking forward, is it possible that the symbolizer might support more than one markup, and would it change the interface? My thoughts are that we could introduce another parameter --filter-markup=<llvm | something-else> which could default to the LLVM Markup syntax, so using `--filter` isn't a problem.
================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/Filter.h:35
+ /// This must be called for each line of the input stream before calls to
+ /// filter() for elements of that line. The provided \p Line must be that
+ /// passed to parseLine() to produce the elements to be later passed to
----------------
Just a subjective opinion, `must be that passed` read a bit abruptly. Perhaps something like:
`must be the same Line that was passed to parseLine() to ...`
================
Comment at: llvm/lib/DebugInfo/Symbolize/Filter.cpp:142
+ for (unsigned I = 0, E = Loc - Line.begin(); I < E; ++I)
+ errs() << ' ';
+ WithColor(errs(), HighlightColor::String) << '^';
----------------
Could be possible to use `indent()` here (https://llvm.org/doxygen/classllvm_1_1raw__ostream.html#a8fdf5cdf041c8aded7e3308c1c3efacc)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126980/new/
https://reviews.llvm.org/D126980
More information about the llvm-commits
mailing list