[PATCH] D124798: [Symbolize] Parse multi-line markup elements.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/Markup.h:64
   /// last use.
   void parseLine(StringRef Line);
 
----------------
Will be useful to expand the comment to indicate the expected usage for multiline tags. It looks like multiple calls to parseLine() are needed with the only information that can be used to determine if we are in the middle of processing a mulitple line case is that nextNode() will return `None` but there is still input remaining?



================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/Markup.h:70
+  /// nextNode() to return additional nodes.
+  void flush();
+
----------------
Could be worth giving an example?
```
/// This allows the parser to finish any deferred processing, such as an in progress Multiline tag, and
/// may cause nextNode() to return additional nodes.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Markup.cpp:73
+      // Begin recording the multi-line element.
+      assert(InProgressMultiline.empty() &&
+             "At most one multi-line element can begin at a time.");
----------------
I think this could occur due to malformed input? For example
```
parseLine("{{{first");
nextElement() returns None
parseLine("}}"); // Typo only two closing braces.
nextElement() returns None
parseLine("{{{second"); // Assert failure "At most one multi-line element can begin at a time"
```
Would imply that an assertions build would crash on malformed input, but would append `{{{second` to the in progress multiline otherwise. I think this would be useful to report as a parse error. An assert seems like it is more appropriate for a coding error (misuse of the API).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124798