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

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h:69
+    // it's from a later mmap.
+    bool IsOriginal;
+
----------------
peter.smith wrote:
> Would `InlineMMaps` be more descriptive? I think this is true when there is a module info line that has a module element immediately followed by mmaps for that module id. False if a new module info line is started for a previously seen module id?  
Refactored this away.


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:78
+    if (tryContextualElement(*Node, Deferred))
+      return true;
+
----------------
peter.smith wrote:
> Was thinking if there was a neat way of making sure that if true then all Deferred nodes had been output. Only thing I could think of was clearing Deferred in the `FlushDeferred` lambda and asserting that it was empty here. It would sadly mean making DeferredNodes non const in `tryContextualElement` so debateable whether it is worth doing.  
We don't necessarily need to output the DeferredNodes if the function returns true. In case of errors, the line is still considered contextual, but it still might be foldable away. In this case, we wouldn't ever call FlushDeferred, but we would still return true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129519