[PATCH] D126980: [Symbolize] Add log markup --filter to llvm-symbolizer.

[Peter Smith via Phabricator via llvm-commits]

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