[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:110
+
+  if (Element.Tag == "mmap") {
+    Optional<MMap> ParsedMMap = parseMMap(Element);
----------------
phosek wrote:
> One potential improvement I've been thinking about would be to split the handling of different tags into separate methods. What I had in mind would be to have an `enum` for known tags, use `llvm:StringSwitch` to convert the tag to an enum and then use it for the dispatch. The only complication is `endModuleInfoLine` which needs a special handling. This could also be done in a future change.
I played around with this a bit; it seems like there's exactly one string comparison per element per known tag, so going through an enum woudn't really add anything from a performance standpoint without building a real lexer and trie-ing-up the tags.

The string-switch approach would be more readable though; along the same lines, I broke up out each tag in tryContextualElement into a try<TAG>() function. Each begins with a tag check, so the end result for tryContextualElement ends up looking more like a usual case analysis in a recursive-descent parser.


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:137
+
+  // Only mmap elements can extend a previous module info line, and this node
+  // cannot be an mmap element.
----------------
peter.smith wrote:
> May be worth making it more explicit that `mmap` needs to be processed before the other tags due to the presence of `endModuleInfoLine` here. Without looking carefully it could seem like new tags can be added anywhere. Some possibilities:
> - Comment in header or by mmap.
> - if (Element.Tag == "mmap") { ... } else { endModuleInfoLine(); ... }
> - if (Element.Tag != "mmap") { endModuleInfoLine(); } else if (ElementTag == "mmap") { ... } if (Element.Tag == "reset") { ... }
Ended up splitting these out into methods, so I've added a comment in the greatly-reduced tryContextualElement().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129519