[PATCH] D129519: [Symbolizer] Implement contextual symbolizer markup elements.

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:45
+  SmallVector<MarkupNode> DeferredNodes;
+  while (Optional<MarkupNode> Node = Parser.nextNode()) {
+    // If this was a contextual line, then summarily stop processing.
----------------
peter.smith wrote:
> I think it would be useful to retain the information in the previous comment
> ```
> // See if the line consists of whitespace and SGR nodes followed by a contextual
> // element. In this case, anything after the contextual element can be elided,
> // and the entire line might be folded away with the next contextual element.
> ```
> Without this `contextual line` looks like it is a term that isn't really defined, other than by guessing from the context. 
Done; updated the text to match the new semantics.


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:53
+
+  // This was not a contextual line.
+  endModuleInfoLine();
----------------
peter.smith wrote:
> I had to look at the definition of `endModuleLine()` to check what happened if there was no module to end. Could be worth breaking the symmetry and changing the name to something like `endAnyModuleInfoLine` which implies that there may not be one? Alternatively // This was not a contextual line. Output any pending ModuleInfo.
I like `endAnyModuleInfoLine`; it captures the notion I was trying to convey better than `endModuleLine`. Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129519