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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for the update, I prefer the deferred nodes approach you have. I've only got a couple of optional suggestions.



================
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.
----------------
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. 


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:53
+
+  // This was not a contextual line.
+  endModuleInfoLine();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129519