[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:120
 
+  // Nodes at the beginning of a possibly-conextual line tha have not yet been
+  // emitted. These may be elided if the line ends up being contextual.
----------------
peter.smith wrote:
> typos: 
> conextual -> contextual
> tha -> that
Removed.


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:110
+
+  if (Element.Tag == "mmap") {
+    Optional<MMap> ParsedMMap = parseMMap(Element);
----------------
phosek wrote:
> 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.
Ah, that's good to know; I had thought it suspicious that StringSwitch even existed, given that it doesn't tend to make the surrounding code that much clearer than if/else. I didn't know that the C++ optimizer could do detailed looks into things like StringRef's, but it makes sense in retrospect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129519