[PATCH] D59049: [Remarks] Add a new Remark / RemarkParser abstraction

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg marked an inline comment as done.
thegameg added inline comments.


================
Comment at: llvm/trunk/lib/Remarks/RemarkParser.cpp:23
 
-namespace {
-struct YAMLRemarkParser {
-  /// Source manager for better error messages.
-  SourceMgr SM;
-  /// Stream for yaml parsing.
-  yaml::Stream Stream;
-  /// Storage for the error stream.
-  std::string ErrorString;
-  /// The error stream.
-  raw_string_ostream ErrorStream;
-  /// Iterator in the YAML stream.
-  yaml::document_iterator DI;
-  /// The parsed remark (if any).
-  Optional<LLVMRemarkEntry> LastRemark;
-  /// Temporary parsing buffer for the arguments.
-  SmallVector<LLVMRemarkArg, 8> TmpArgs;
-  /// The state used by the parser to parse a remark entry. Invalidated with
-  /// every call to `parseYAMLElement`.
-  struct ParseState {
-    /// Temporary parsing buffer for the arguments.
-    SmallVectorImpl<LLVMRemarkArg> *Args;
-    StringRef Type;
-    StringRef Pass;
-    StringRef Name;
-    StringRef Function;
-    /// Optional.
-    Optional<StringRef> File;
-    Optional<unsigned> Line;
-    Optional<unsigned> Column;
-    Optional<unsigned> Hotness;
-
-    ParseState(SmallVectorImpl<LLVMRemarkArg> &Args) : Args(&Args) {}
-    /// Use Args only as a **temporary** buffer.
-    ~ParseState() { Args->clear(); }
-  };
-
-  ParseState State;
-
-  /// Set to `true` if we had any errors during parsing.
-  bool HadAnyErrors = false;
-
-  YAMLRemarkParser(StringRef Buf)
-      : SM(), Stream(Buf, SM), ErrorString(), ErrorStream(ErrorString),
-        DI(Stream.begin()), LastRemark(), TmpArgs(), State(TmpArgs) {
-    SM.setDiagHandler(YAMLRemarkParser::HandleDiagnostic, this);
-  }
-
-  /// Parse a YAML element.
-  Error parseYAMLElement(yaml::Document &Remark);
-
-private:
-  /// Parse one key to a string.
-  /// otherwise.
-  Error parseKey(StringRef &Result, yaml::KeyValueNode &Node);
-  /// Parse one value to a string.
-  Error parseValue(StringRef &Result, yaml::KeyValueNode &Node);
-  /// Parse one value to an unsigned.
-  Error parseValue(Optional<unsigned> &Result, yaml::KeyValueNode &Node);
-  /// Parse a debug location.
-  Error parseDebugLoc(Optional<StringRef> &File, Optional<unsigned> &Line,
-                      Optional<unsigned> &Column, yaml::KeyValueNode &Node);
-  /// Parse an argument.
-  Error parseArg(SmallVectorImpl<LLVMRemarkArg> &TmpArgs, yaml::Node &Node);
-
-  /// Handle a diagnostic from the YAML stream. Records the error in the
-  /// YAMLRemarkParser class.
-  static void HandleDiagnostic(const SMDiagnostic &Diag, void *Ctx) {
-    assert(Ctx && "Expected non-null Ctx in diagnostic handler.");
-    auto *Parser = static_cast<YAMLRemarkParser *>(Ctx);
-    Diag.print(/*ProgName=*/nullptr, Parser->ErrorStream, /*ShowColors*/ false,
-               /*ShowKindLabels*/ true);
-  }
-};
-
-class ParseError : public ErrorInfo<ParseError> {
-public:
-  static char ID;
-
-  ParseError(StringRef Message, yaml::Node &Node)
-      : Message(Message), Node(Node) {}
-
-  void log(raw_ostream &OS) const override { OS << Message; }
-  std::error_code convertToErrorCode() const override {
-    return inconvertibleErrorCode();
-  }
-
-  StringRef getMessage() const { return Message; }
-  yaml::Node &getNode() const { return Node; }
-
-private:
-  StringRef Message; // No need to hold a full copy of the buffer.
-  yaml::Node &Node;
-};
-
-char ParseError::ID = 0;
-
-static LLVMRemarkStringRef toRemarkStr(StringRef Str) {
-  return {Str.data(), static_cast<uint32_t>(Str.size())};
-}
+Parser::Parser(StringRef Buf) : Impl(llvm::make_unique<YAMLParserImpl>(Buf)) {}
 
----------------
rupprecht wrote:
> Because ParserImpl doesn't have a virtual destructor, ~ParserImpl() is called instead of ~YAMLParserImpl() in ~Parser(). (At least that's my understanding of the error).
> 
> We noticed these failures when running YAMLRemarksParsingTest with asan. It should be fixed by rL356583.
Good catch, thank you for fixing it!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59049