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

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:110
+
+  if (Element.Tag == "mmap") {
+    Optional<MMap> ParsedMMap = parseMMap(Element);
----------------
mysterymath wrote:
> 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.
FYI the code generated by `llvm::StringSwitch` is more efficient than plain string comparison, the pattern used by `llvm::StringSwitch` implementation was chosen deliberately to be recognized by the optimizer. However, I don't think the string comparisons used here are performance critical so the difference is probably going to be in the noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129519