[Remarks] Simplify and refactor the RemarkParser interface

Before, everything was based on some kind of type erased parser
implementation which container a lot of boilerplate code when multiple
formats were to be supported.

This simplifies it by:

* the remark now owns its arguments
* *always* returning an error from the implementation side
* working around the way the YAML parser reports errors: catch them through
callbacks and re-insert them in a proper llvm::Error
* add a CParser wrapper that is used when implementing the C API to
avoid cluttering the C++ API with useless state
* LLVMRemarkParserGetNext now returns an object that needs to be
released to avoid leaking resources
* add a new API to dispose of a remark entry: LLVMRemarkEntryDispose

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@366217 91177308-0d34-0410-b5e6-96231b3b80d8

docs/Remarks.rst

@@ -295,6 +295,7 @@ The typical usage through the C API is like the following:
     LLVMRemarkEntryRef Remark = NULL;
     while ((Remark = LLVMRemarkParserGetNext(Parser))) {
        // use Remark
+       LLVMRemarkEntryDispose(Remark); // Release memory.
     }
     bool HasError = LLVMRemarkParserHasError(Parser);
     LLVMRemarkParserDispose(Parser);

include/llvm-c/Remarks.h

@@ -136,6 +136,13 @@ extern LLVMRemarkDebugLocRef LLVMRemarkArgGetDebugLoc(LLVMRemarkArgRef Arg);
  */
 typedef struct LLVMRemarkOpaqueEntry *LLVMRemarkEntryRef;
 
+/**
+ * Free the resources used by the remark entry.
+ *
+ * \since REMARKS_API_VERSION=0
+ */
+extern void LLVMRemarkEntryDispose(LLVMRemarkEntryRef Remark);
+
 /**
  * The type of the remark. For example, it can allow users to only keep the
  * missed optimizations from the compiler.
@@ -161,7 +168,7 @@ extern LLVMRemarkStringRef
 LLVMRemarkEntryGetRemarkName(LLVMRemarkEntryRef Remark);
 
 /**
- * Get the name of the function being processsed when the remark was emitted.
+ * Get the name of the function being processed when the remark was emitted.
  *
  * \since REMARKS_API_VERSION=0
  */
@@ -199,6 +206,8 @@ extern uint32_t LLVMRemarkEntryGetNumArgs(LLVMRemarkEntryRef Remark);
  *
  * If there are no arguments in \p Remark, the return value will be `NULL`.
  *
+ * The lifetime of the returned value is bound to the lifetime of \p Remark.
+ *
  * \since REMARKS_API_VERSION=0
  */
 extern LLVMRemarkArgRef LLVMRemarkEntryGetFirstArg(LLVMRemarkEntryRef Remark);
@@ -208,6 +217,8 @@ extern LLVMRemarkArgRef LLVMRemarkEntryGetFirstArg(LLVMRemarkEntryRef Remark);
  *
  * Returns `NULL` if there are no more arguments available.
  *
+ * The lifetime of the returned value is bound to the lifetime of \p Remark.
+ *
  * \since REMARKS_API_VERSION=0
  */
 extern LLVMRemarkArgRef LLVMRemarkEntryGetNextArg(LLVMRemarkArgRef It,
@@ -232,8 +243,11 @@ extern LLVMRemarkParserRef LLVMRemarkParserCreateYAML(const void *Buf,
 /**
  * Returns the next remark in the file.
  *
- * The value pointed to by the return value is invalidated by the next call to
- * LLVMRemarkParserGetNext().
+ * The value pointed to by the return value needs to be disposed using a call to
+ * LLVMRemarkEntryDispose().
+ *
+ * All the entries in the returned value that are of LLVMRemarkStringRef type
+ * will become invalidated once a call to LLVMRemarkParserDispose is made.
  *
  * If the parser reaches the end of the buffer, the return value will be `NULL`.
  *
@@ -258,8 +272,9 @@ extern LLVMRemarkParserRef LLVMRemarkParserCreateYAML(const void *Buf,
  * ```
  * LLVMRemarkParserRef Parser = LLVMRemarkParserCreateYAML(Buf, Size);
  * LLVMRemarkEntryRef Remark = NULL;
- * while ((Remark == LLVMRemarkParserGetNext(Parser))) {
+ * while ((Remark = LLVMRemarkParserGetNext(Parser))) {
  *    // use Remark
+ *    LLVMRemarkEntryDispose(Remark); // Release memory.
  * }
  * bool HasError = LLVMRemarkParserHasError(Parser);
  * LLVMRemarkParserDispose(Parser);

include/llvm/IR/RemarkStreamer.h

@@ -32,15 +32,9 @@ class RemarkStreamer {
   /// The object used to serialize the remarks to a specific format.
   std::unique_ptr<remarks::Serializer> Serializer;
 
-  /// Temporary buffer for converting diagnostics into remark objects. This is
-  /// used for the remark arguments that are converted from a vector of
-  /// diagnostic arguments to a vector of remark arguments.
-  SmallVector<remarks::Argument, 8> TmpArgs;
-  /// Convert diagnostics into remark objects. The result uses \p TmpArgs as a
-  /// temporary buffer for the remark arguments, and relies on all the strings
-  /// to be kept in memory until the next call to `toRemark`.
-  /// The lifetime of the members of the result is bound to the lifetime of both
-  /// the remark streamer and the LLVM diagnostics.
+  /// Convert diagnostics into remark objects.
+  /// The lifetime of the members of the result is bound to the lifetime of
+  /// the LLVM diagnostics.
   remarks::Remark toRemark(const DiagnosticInfoOptimizationBase &Diag);
 
 public:

include/llvm/Remarks/Remark.h

@@ -85,10 +85,23 @@ struct Remark {
   Optional<uint64_t> Hotness;
 
   /// Arguments collected via the streaming interface.
-  ArrayRef<Argument> Args;
+  SmallVector<Argument, 5> Args;
+
+  Remark() = default;
+  Remark(Remark &&) = default;
+  Remark &operator=(Remark &&) = default;
 
   /// Return a message composed from the arguments as a string.
   std::string getArgsAsMsg() const;
+
+  /// Clone this remark to explicitly ask for a copy.
+  Remark clone() const { return *this; }
+
+private:
+  /// In order to avoid unwanted copies, "delete" the copy constructor.
+  /// If a copy is needed, it should be done through `Remark::clone()`.
+  Remark(const Remark &) = default;
+  Remark& operator=(const Remark &) = default;
 };
 
 // Create wrappers for C Binding types (see CBindingWrapping.h).

include/llvm/Remarks/RemarkParser.h

@@ -26,27 +26,33 @@ namespace remarks {
 struct ParserImpl;
 struct ParsedStringTable;
 
+class EndOfFileError : public ErrorInfo<EndOfFileError> {
+public:
+  static char ID;
+
+  EndOfFileError() {}
+
+  void log(raw_ostream &OS) const override { OS << "End of file reached."; }
+  std::error_code convertToErrorCode() const override {
+    return inconvertibleErrorCode();
+  }
+};
+
 /// Parser used to parse a raw buffer to remarks::Remark objects.
 struct Parser {
-  /// The hidden implementation of the parser.
-  std::unique_ptr<ParserImpl> Impl;
-
-  /// Create a parser parsing \p Buffer to Remark objects.
-  /// This constructor should be only used for parsing remarks without a string
-  /// table.
-  Parser(Format ParserFormat, StringRef Buffer);
+  /// The format of the parser.
+  Format ParserFormat;
 
-  /// Create a parser parsing \p Buffer to Remark objects, using \p StrTab as a
-  /// string table.
-  Parser(Format ParserFormat, StringRef Buffer,
-         const ParsedStringTable &StrTab);
+  Parser(Format ParserFormat) : ParserFormat(ParserFormat) {}
 
-  // Needed because ParserImpl is an incomplete type.
-  ~Parser();
+  /// If no error occurs, this returns a valid Remark object.
+  /// If an error of type EndOfFileError occurs, it is safe to recover from it
+  /// by stopping the parsing.
+  /// If any other error occurs, it should be propagated to the user.
+  /// The pointer should never be null.
+  virtual Expected<std::unique_ptr<Remark>> next() = 0;
 
-  /// Returns an empty Optional if it reached the end.
-  /// Returns a valid remark otherwise.
-  Expected<const Remark *> getNext() const;
+  virtual ~Parser() = default;
 };
 
 /// In-memory representation of the string table parsed from a buffer (e.g. the
@@ -61,6 +67,10 @@ struct ParsedStringTable {
   ParsedStringTable(StringRef Buffer);
 };
 
+Expected<std::unique_ptr<Parser>>
+createRemarkParser(Format ParserFormat, StringRef Buf,
+                   Optional<const ParsedStringTable *> StrTab = None);
+
 } // end namespace remarks
 } // end namespace llvm
 

include/llvm/Support/SourceMgr.h

@@ -106,6 +106,8 @@ public:
   SourceMgr() = default;
   SourceMgr(const SourceMgr &) = delete;
   SourceMgr &operator=(const SourceMgr &) = delete;
+  SourceMgr(SourceMgr &&) = default;
+  SourceMgr &operator=(SourceMgr &&) = default;
   ~SourceMgr() = default;
 
   void setIncludeDirs(const std::vector<std::string> &Dirs) {

lib/IR/RemarkStreamer.cpp

@@ -72,9 +72,6 @@ toRemarkLocation(const DiagnosticLocation &DL) {
 /// LLVM Diagnostic -> Remark
 remarks::Remark
 RemarkStreamer::toRemark(const DiagnosticInfoOptimizationBase &Diag) {
-  // Re-use the buffer.
-  TmpArgs.clear();
-
   remarks::Remark R; // The result.
   R.RemarkType = toRemarkType(static_cast<DiagnosticKind>(Diag.getKind()));
   R.PassName = Diag.getPassName();
@@ -84,15 +81,12 @@ RemarkStreamer::toRemark(const DiagnosticInfoOptimizationBase &Diag) {
   R.Loc = toRemarkLocation(Diag.getLocation());
   R.Hotness = Diag.getHotness();
 
-  // Use TmpArgs to build the list of arguments and re-use the memory allocated
-  // from previous remark conversions.
   for (const DiagnosticInfoOptimizationBase::Argument &Arg : Diag.getArgs()) {
-    TmpArgs.emplace_back();
-    TmpArgs.back().Key = Arg.Key;
-    TmpArgs.back().Val = Arg.Val;
-    TmpArgs.back().Loc = toRemarkLocation(Arg.Loc);
+    R.Args.emplace_back();
+    R.Args.back().Key = Arg.Key;
+    R.Args.back().Val = Arg.Val;
+    R.Args.back().Loc = toRemarkLocation(Arg.Loc);
   }
-  R.Args = TmpArgs; // This is valid until the next call to this function.
 
   return R;
 }

lib/Remarks/Remark.cpp

@@ -66,6 +66,10 @@ LLVMRemarkArgGetDebugLoc(LLVMRemarkArgRef Arg) {
   return nullptr;
 }
 
+extern "C" void LLVMRemarkEntryDispose(LLVMRemarkEntryRef Remark) {
+  delete unwrap(Remark);
+}
+
 extern "C" LLVMRemarkType LLVMRemarkEntryGetType(LLVMRemarkEntryRef Remark) {
   // Assume here that the enums can be converted both ways.
   return static_cast<LLVMRemarkType>(unwrap(Remark)->RemarkType);

lib/Remarks/RemarkParser.cpp

@@ -20,69 +20,7 @@
 using namespace llvm;
 using namespace llvm::remarks;
 
-static std::unique_ptr<ParserImpl> formatToParserImpl(Format ParserFormat,
-                                                      StringRef Buf) {
-  switch (ParserFormat) {
-  case Format::YAML:
-    return llvm::make_unique<YAMLParserImpl>(Buf);
-  case Format::Unknown:
-    llvm_unreachable("Unhandled llvm::remarks::ParserFormat enum");
-    return nullptr;
-  };
-}
-
-static std::unique_ptr<ParserImpl>
-formatToParserImpl(Format ParserFormat, StringRef Buf,
-                   const ParsedStringTable &StrTab) {
-  switch (ParserFormat) {
-  case Format::YAML:
-    return llvm::make_unique<YAMLParserImpl>(Buf, &StrTab);
-  case Format::Unknown:
-    llvm_unreachable("Unhandled llvm::remarks::ParserFormat enum");
-    return nullptr;
-  };
-}
-
-Parser::Parser(Format ParserFormat, StringRef Buf)
-    : Impl(formatToParserImpl(ParserFormat, Buf)) {}
-
-Parser::Parser(Format ParserFormat, StringRef Buf,
-               const ParsedStringTable &StrTab)
-    : Impl(formatToParserImpl(ParserFormat, Buf, StrTab)) {}
-
-Parser::~Parser() = default;
-
-static Expected<const Remark *> getNextYAML(YAMLParserImpl &Impl) {
-  YAMLRemarkParser &YAMLParser = Impl.YAMLParser;
-  // Check for EOF.
-  if (Impl.YAMLIt == Impl.YAMLParser.Stream.end())
-    return nullptr;
-
-  auto CurrentIt = Impl.YAMLIt;
-
-  // Try to parse an entry.
-  if (Error E = YAMLParser.parseYAMLElement(*CurrentIt)) {
-    // Set the iterator to the end, in case the user calls getNext again.
-    Impl.YAMLIt = Impl.YAMLParser.Stream.end();
-    return std::move(E);
-  }
-
-  // Move on.
-  ++Impl.YAMLIt;
-
-  // Return the just-parsed remark.
-  if (const Optional<YAMLRemarkParser::ParseState> &State = YAMLParser.State)
-    return &State->TheRemark;
-  else
-    return createStringError(std::make_error_code(std::errc::invalid_argument),
-                             "unexpected error while parsing.");
-}
-
-Expected<const Remark *> Parser::getNext() const {
-  if (auto *Impl = dyn_cast<YAMLParserImpl>(this->Impl.get()))
-    return getNextYAML(*Impl);
-  llvm_unreachable("Get next called with an unknown parsing implementation.");
-}
+char EndOfFileError::ID = 0;
 
 ParsedStringTable::ParsedStringTable(StringRef InBuffer) : Buffer(InBuffer) {
   while (!InBuffer.empty()) {
@@ -109,59 +47,70 @@ Expected<StringRef> ParsedStringTable::operator[](size_t Index) const {
   return StringRef(Buffer.data() + Offset, NextOffset - Offset - 1);
 }
 
+Expected<std::unique_ptr<Parser>>
+llvm::remarks::createRemarkParser(Format ParserFormat, StringRef Buf,
+                                  Optional<const ParsedStringTable *> StrTab) {
+  switch (ParserFormat) {
+  case Format::YAML:
+    return llvm::make_unique<YAMLRemarkParser>(Buf, StrTab);
+  case Format::Unknown:
+    return createStringError(std::make_error_code(std::errc::invalid_argument),
+                             "Unknown remark parser format.");
+  }
+}
+
+// Wrapper that holds the state needed to interact with the C API.
+struct CParser {
+  std::unique_ptr<Parser> TheParser;
+  Optional<std::string> Err;
+
+  CParser(Format ParserFormat, StringRef Buf,
+          Optional<const ParsedStringTable *> StrTab = None)
+      : TheParser(cantFail(createRemarkParser(ParserFormat, Buf, StrTab))) {}
+
+  void handleError(Error E) { Err.emplace(toString(std::move(E))); }
+  bool hasError() const { return Err.hasValue(); }
+  const char *getMessage() const { return Err ? Err->c_str() : nullptr; };
+};
+
 // Create wrappers for C Binding types (see CBindingWrapping.h).
-DEFINE_SIMPLE_CONVERSION_FUNCTIONS(remarks::Parser, LLVMRemarkParserRef)
+DEFINE_SIMPLE_CONVERSION_FUNCTIONS(CParser, LLVMRemarkParserRef)
 
 extern "C" LLVMRemarkParserRef LLVMRemarkParserCreateYAML(const void *Buf,
                                                           uint64_t Size) {
-  return wrap(new remarks::Parser(
-      remarks::Format::YAML, StringRef(static_cast<const char *>(Buf), Size)));
-}
-
-static void handleYAMLError(remarks::YAMLParserImpl &Impl, Error E) {
-  handleAllErrors(
-      std::move(E),
-      [&](const YAMLParseError &PE) {
-        Impl.YAMLParser.Stream.printError(&PE.getNode(),
-                                          Twine(PE.getMessage()) + Twine('\n'));
-      },
-      [&](const ErrorInfoBase &EIB) { EIB.log(Impl.YAMLParser.ErrorStream); });
-  Impl.HasErrors = true;
+  return wrap(new CParser(Format::YAML,
+                          StringRef(static_cast<const char *>(Buf), Size)));
 }
 
 extern "C" LLVMRemarkEntryRef
 LLVMRemarkParserGetNext(LLVMRemarkParserRef Parser) {
-  remarks::Parser &TheParser = *unwrap(Parser);
-
-  Expected<const remarks::Remark *> RemarkOrErr = TheParser.getNext();
-  if (!RemarkOrErr) {
-    // Error during parsing.
-    if (auto *Impl = dyn_cast<remarks::YAMLParserImpl>(TheParser.Impl.get()))
-      handleYAMLError(*Impl, RemarkOrErr.takeError());
-    else
-      llvm_unreachable("unkown parser implementation.");
+  CParser &TheCParser = *unwrap(Parser);
+  remarks::Parser &TheParser = *TheCParser.TheParser;
+
+  Expected<std::unique_ptr<Remark>> MaybeRemark = TheParser.next();
+  if (Error E = MaybeRemark.takeError()) {
+    if (E.isA<EndOfFileError>()) {
+      consumeError(std::move(E));
+      return nullptr;
+    }
+
+    // Handle the error. Allow it to be checked through HasError and
+    // GetErrorMessage.
+    TheCParser.handleError(std::move(E));
     return nullptr;
   }
 
-  if (*RemarkOrErr == nullptr)
-    return nullptr;
   // Valid remark.
-  return wrap(*RemarkOrErr);
+  return wrap(MaybeRemark->release());
 }
 
 extern "C" LLVMBool LLVMRemarkParserHasError(LLVMRemarkParserRef Parser) {
-  if (auto *Impl =
-          dyn_cast<remarks::YAMLParserImpl>(unwrap(Parser)->Impl.get()))
-    return Impl->HasErrors;
-  llvm_unreachable("unkown parser implementation.");
+  return unwrap(Parser)->hasError();
 }
 
 extern "C" const char *
 LLVMRemarkParserGetErrorMessage(LLVMRemarkParserRef Parser) {
-  if (auto *Impl =
-          dyn_cast<remarks::YAMLParserImpl>(unwrap(Parser)->Impl.get()))
-    return Impl->YAMLParser.ErrorStream.str().c_str();
-  llvm_unreachable("unkown parser implementation.");
+  return unwrap(Parser)->getMessage();
 }
 
 extern "C" void LLVMRemarkParserDispose(LLVMRemarkParserRef Parser) {

lib/Remarks/RemarkParserImpl.h

@@ -1,33 +0,0 @@
-//===-- RemarkParserImpl.h - Implementation details -------------*- C++/-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file provides implementation details for the remark parser.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLVM_REMARKS_REMARK_PARSER_IMPL_H
-#define LLVM_REMARKS_REMARK_PARSER_IMPL_H
-
-#include "llvm/Remarks/RemarkParser.h"
-
-namespace llvm {
-namespace remarks {
-/// This is used as a base for any parser implementation.
-struct ParserImpl {
-  explicit ParserImpl(Format ParserFormat) : ParserFormat(ParserFormat) {}
-  // Virtual destructor prevents mismatched deletes
-  virtual ~ParserImpl() {}
-
-  // The parser format. This is used as a tag to safely cast between
-  // implementations.
-  Format ParserFormat;
-};
-} // end namespace remarks
-} // end namespace llvm
-
-#endif /* LLVM_REMARKS_REMARK_PARSER_IMPL_H */

lib/Remarks/YAMLRemarkParser.cpp

@@ -20,255 +20,308 @@ using namespace llvm::remarks;
 
 char YAMLParseError::ID = 0;
 
-Error YAMLRemarkParser::parseKey(StringRef &Result, yaml::KeyValueNode &Node) {
-  if (auto *Key = dyn_cast<yaml::ScalarNode>(Node.getKey())) {
-    Result = Key->getRawValue();
+static void handleDiagnostic(const SMDiagnostic &Diag, void *Ctx) {
+  assert(Ctx && "Expected non-null Ctx in diagnostic handler.");
+  std::string &Message = *static_cast<std::string *>(Ctx);
+  assert(Message.empty() && "Expected an empty string.");
+  raw_string_ostream OS(Message);
+  Diag.print(/*ProgName=*/nullptr, OS, /*ShowColors*/ false,
+             /*ShowKindLabels*/ true);
+  OS << '\n';
+  OS.flush();
+}
+
+YAMLParseError::YAMLParseError(StringRef Msg, SourceMgr &SM,
+                               yaml::Stream &Stream, yaml::Node &Node) {
+  // 1) Set up a diagnostic handler to avoid errors being printed out to
+  // stderr.
+  // 2) Use the stream to print the error with the associated node.
+  // 3) The stream will use the source manager to print the error, which will
+  // call the diagnostic handler.
+  // 4) The diagnostic handler will stream the error directly into this object's
+  // Message member, which is used when logging is asked for.
+  auto OldDiagHandler = SM.getDiagHandler();
+  auto OldDiagCtx = SM.getDiagContext();
+  SM.setDiagHandler(handleDiagnostic, &Message);
+  Stream.printError(&Node, Twine(Msg) + Twine('\n'));
+  // Restore the old handlers.
+  SM.setDiagHandler(OldDiagHandler, OldDiagCtx);
+}
+
+static SourceMgr setupSM(std::string &LastErrorMessage) {
+  SourceMgr SM;
+  SM.setDiagHandler(handleDiagnostic, &LastErrorMessage);
+  return SM;
+}
+
+YAMLRemarkParser::YAMLRemarkParser(StringRef Buf,
+                                   Optional<const ParsedStringTable *> StrTab)
+    : Parser{Format::YAML}, StrTab(StrTab), LastErrorMessage(),
+      SM(setupSM(LastErrorMessage)), Stream(Buf, SM), YAMLIt(Stream.begin()) {}
+
+Error YAMLRemarkParser::error(StringRef Message, yaml::Node &Node) {
+  return make_error<YAMLParseError>(Message, SM, Stream, Node);
+}
+
+Error YAMLRemarkParser::error() {
+  if (LastErrorMessage.empty())
     return Error::success();
+  Error E = make_error<YAMLParseError>(LastErrorMessage);
+  LastErrorMessage.clear();
+  return E;
+}
+
+Expected<std::unique_ptr<Remark>>
+YAMLRemarkParser::parseRemark(yaml::Document &RemarkEntry) {
+  if (Error E = error())
+    return std::move(E);
+
+  yaml::Node *YAMLRoot = RemarkEntry.getRoot();
+  if (!YAMLRoot) {
+    return createStringError(std::make_error_code(std::errc::invalid_argument),
+                             "not a valid YAML file.");
   }
 
-  return make_error<YAMLParseError>("key is not a string.", Node);
+  auto *Root = dyn_cast<yaml::MappingNode>(YAMLRoot);
+  if (!Root)
+    return error("document root is not of mapping type.", *YAMLRoot);
+
+  std::unique_ptr<Remark> Result = llvm::make_unique<Remark>();
+  Remark &TheRemark = *Result;
+
+  // First, the type. It needs special handling since is not part of the
+  // key-value stream.
+  Expected<Type> T = parseType(*Root);
+  if (!T)
+    return T.takeError();
+  else
+    TheRemark.RemarkType = *T;
+
+  // Then, parse the fields, one by one.
+  for (yaml::KeyValueNode &RemarkField : *Root) {
+    Expected<StringRef> MaybeKey = parseKey(RemarkField);
+    if (!MaybeKey)
+      return MaybeKey.takeError();
+    StringRef KeyName = *MaybeKey;
+
+    if (KeyName == "Pass") {
+      if (Expected<StringRef> MaybeStr = parseStr(RemarkField))
+        TheRemark.PassName = *MaybeStr;
+      else
+        return MaybeStr.takeError();
+    } else if (KeyName == "Name") {
+      if (Expected<StringRef> MaybeStr = parseStr(RemarkField))
+        TheRemark.RemarkName = *MaybeStr;
+      else
+        return MaybeStr.takeError();
+    } else if (KeyName == "Function") {
+      if (Expected<StringRef> MaybeStr = parseStr(RemarkField))
+        TheRemark.FunctionName = *MaybeStr;
+      else
+        return MaybeStr.takeError();
+    } else if (KeyName == "Hotness") {
+      if (Expected<unsigned> MaybeU = parseUnsigned(RemarkField))
+        TheRemark.Hotness = *MaybeU;
+      else
+        return MaybeU.takeError();
+    } else if (KeyName == "DebugLoc") {
+      if (Expected<RemarkLocation> MaybeLoc = parseDebugLoc(RemarkField))
+        TheRemark.Loc = *MaybeLoc;
+      else
+        return MaybeLoc.takeError();
+    } else if (KeyName == "Args") {
+      auto *Args = dyn_cast<yaml::SequenceNode>(RemarkField.getValue());
+      if (!Args)
+        return error("wrong value type for key.", RemarkField);
+
+      for (yaml::Node &Arg : *Args) {
+        if (Expected<Argument> MaybeArg = parseArg(Arg))
+          TheRemark.Args.push_back(*MaybeArg);
+        else
+          return MaybeArg.takeError();
+      }
+    } else {
+      return error("unknown key.", RemarkField);
+    }
+  }
+
+  // Check if any of the mandatory fields are missing.
+  if (TheRemark.RemarkType == Type::Unknown || TheRemark.PassName.empty() ||
+      TheRemark.RemarkName.empty() || TheRemark.FunctionName.empty())
+    return error("Type, Pass, Name or Function missing.",
+                 *RemarkEntry.getRoot());
+
+  return std::move(Result);
 }
 
-template <typename T>
-Error YAMLRemarkParser::parseStr(T &Result, yaml::KeyValueNode &Node) {
+Expected<Type> YAMLRemarkParser::parseType(yaml::MappingNode &Node) {
+  auto Type = StringSwitch<remarks::Type>(Node.getRawTag())
+                  .Case("!Passed", remarks::Type::Passed)
+                  .Case("!Missed", remarks::Type::Missed)
+                  .Case("!Analysis", remarks::Type::Analysis)
+                  .Case("!AnalysisFPCommute", remarks::Type::AnalysisFPCommute)
+                  .Case("!AnalysisAliasing", remarks::Type::AnalysisAliasing)
+                  .Case("!Failure", remarks::Type::Failure)
+                  .Default(remarks::Type::Unknown);
+  if (Type == remarks::Type::Unknown)
+    return error("expected a remark tag.", Node);
+  return Type;
+}
+
+Expected<StringRef> YAMLRemarkParser::parseKey(yaml::KeyValueNode &Node) {
+  if (auto *Key = dyn_cast<yaml::ScalarNode>(Node.getKey()))
+    return Key->getRawValue();
+
+  return error("key is not a string.", Node);
+}
+
+Expected<StringRef> YAMLRemarkParser::parseStr(yaml::KeyValueNode &Node) {
   auto *Value = dyn_cast<yaml::ScalarNode>(Node.getValue());
   if (!Value)
-    return make_error<YAMLParseError>("expected a value of scalar type.", Node);
-  StringRef Tmp;
+    return error("expected a value of scalar type.", Node);
+  StringRef Result;
   if (!StrTab) {
-    Tmp = Value->getRawValue();
+    Result = Value->getRawValue();
   } else {
     // If we have a string table, parse it as an unsigned.
     unsigned StrID = 0;
-    if (Error E = parseUnsigned(StrID, Node))
-      return E;
+    if (Expected<unsigned> MaybeStrID = parseUnsigned(Node))
+      StrID = *MaybeStrID;
+    else
+      return MaybeStrID.takeError();
+
     if (Expected<StringRef> Str = (**StrTab)[StrID])
-      Tmp = *Str;
+      Result = *Str;
     else
       return Str.takeError();
   }
 
-  if (Tmp.front() == '\'')
-    Tmp = Tmp.drop_front();
-
-  if (Tmp.back() == '\'')
-    Tmp = Tmp.drop_back();
+  if (Result.front() == '\'')
+    Result = Result.drop_front();
 
-  Result = Tmp;
+  if (Result.back() == '\'')
+    Result = Result.drop_back();
 
-  return Error::success();
+  return Result;
 }
 
-template <typename T>
-Error YAMLRemarkParser::parseUnsigned(T &Result, yaml::KeyValueNode &Node) {
+Expected<unsigned> YAMLRemarkParser::parseUnsigned(yaml::KeyValueNode &Node) {
   SmallVector<char, 4> Tmp;
   auto *Value = dyn_cast<yaml::ScalarNode>(Node.getValue());
   if (!Value)
-    return make_error<YAMLParseError>("expected a value of scalar type.", Node);
+    return error("expected a value of scalar type.", Node);
   unsigned UnsignedValue = 0;
   if (Value->getValue(Tmp).getAsInteger(10, UnsignedValue))
-    return make_error<YAMLParseError>("expected a value of integer type.",
-                                      *Value);
-  Result = UnsignedValue;
-  return Error::success();
-}
-
-Error YAMLRemarkParser::parseType(Type &Result, yaml::MappingNode &Node) {
-  auto Type = StringSwitch<remarks::Type>(Node.getRawTag())
-                  .Case("!Passed", remarks::Type::Passed)
-                  .Case("!Missed", remarks::Type::Missed)
-                  .Case("!Analysis", remarks::Type::Analysis)
-                  .Case("!AnalysisFPCommute", remarks::Type::AnalysisFPCommute)
-                  .Case("!AnalysisAliasing", remarks::Type::AnalysisAliasing)
-                  .Case("!Failure", remarks::Type::Failure)
-                  .Default(remarks::Type::Unknown);
-  if (Type == remarks::Type::Unknown)
-    return make_error<YAMLParseError>("expected a remark tag.", Node);
-  Result = Type;
-  return Error::success();
+    return error("expected a value of integer type.", *Value);
+  return UnsignedValue;
 }
 
-Error YAMLRemarkParser::parseDebugLoc(Optional<RemarkLocation> &Result,
-                                      yaml::KeyValueNode &Node) {
+Expected<RemarkLocation>
+YAMLRemarkParser::parseDebugLoc(yaml::KeyValueNode &Node) {
   auto *DebugLoc = dyn_cast<yaml::MappingNode>(Node.getValue());
   if (!DebugLoc)
-    return make_error<YAMLParseError>("expected a value of mapping type.",
-                                      Node);
+    return error("expected a value of mapping type.", Node);
 
   Optional<StringRef> File;
   Optional<unsigned> Line;
   Optional<unsigned> Column;
 
   for (yaml::KeyValueNode &DLNode : *DebugLoc) {
-    StringRef KeyName;
-    if (Error E = parseKey(KeyName, DLNode))
-      return E;
+    Expected<StringRef> MaybeKey = parseKey(DLNode);
+    if (!MaybeKey)
+      return MaybeKey.takeError();
+    StringRef KeyName = *MaybeKey;
+
     if (KeyName == "File") {
-      if (Error E = parseStr(File, DLNode))
-        return E;
+      if (Expected<StringRef> MaybeStr = parseStr(DLNode))
+        File = *MaybeStr;
+      else
+        return MaybeStr.takeError();
     } else if (KeyName == "Column") {
-      if (Error E = parseUnsigned(Column, DLNode))
-        return E;
+      if (Expected<unsigned> MaybeU = parseUnsigned(DLNode))
+        Column = *MaybeU;
+      else
+        return MaybeU.takeError();
     } else if (KeyName == "Line") {
-      if (Error E = parseUnsigned(Line, DLNode))
-        return E;
+      if (Expected<unsigned> MaybeU = parseUnsigned(DLNode))
+        Line = *MaybeU;
+      else
+        return MaybeU.takeError();
     } else {
-      return make_error<YAMLParseError>("unknown entry in DebugLoc map.",
-                                        DLNode);
+      return error("unknown entry in DebugLoc map.", DLNode);
     }
   }
 
   // If any of the debug loc fields is missing, return an error.
   if (!File || !Line || !Column)
-    return make_error<YAMLParseError>("DebugLoc node incomplete.", Node);
-
-  Result = RemarkLocation{*File, *Line, *Column};
-
-  return Error::success();
-}
-
-Error YAMLRemarkParser::parseRemarkField(yaml::KeyValueNode &RemarkField) {
-
-  StringRef KeyName;
-  if (Error E = parseKey(KeyName, RemarkField))
-    return E;
-
-  if (KeyName == "Pass") {
-    if (Error E = parseStr(State->TheRemark.PassName, RemarkField))
-      return E;
-  } else if (KeyName == "Name") {
-    if (Error E = parseStr(State->TheRemark.RemarkName, RemarkField))
-      return E;
-  } else if (KeyName == "Function") {
-    if (Error E = parseStr(State->TheRemark.FunctionName, RemarkField))
-      return E;
-  } else if (KeyName == "Hotness") {
-    State->TheRemark.Hotness = 0;
-    if (Error E = parseUnsigned(*State->TheRemark.Hotness, RemarkField))
-      return E;
-  } else if (KeyName == "DebugLoc") {
-    if (Error E = parseDebugLoc(State->TheRemark.Loc, RemarkField))
-      return E;
-  } else if (KeyName == "Args") {
-    auto *Args = dyn_cast<yaml::SequenceNode>(RemarkField.getValue());
-    if (!Args)
-      return make_error<YAMLParseError>("wrong value type for key.",
-                                        RemarkField);
-
-    for (yaml::Node &Arg : *Args)
-      if (Error E = parseArg(State->Args, Arg))
-        return E;
-
-    State->TheRemark.Args = State->Args;
-  } else {
-    return make_error<YAMLParseError>("unknown key.", RemarkField);
-  }
+    return error("DebugLoc node incomplete.", Node);
 
-  return Error::success();
+  return RemarkLocation{*File, *Line, *Column};
 }
 
-Error YAMLRemarkParser::parseArg(SmallVectorImpl<Argument> &Args,
-                                 yaml::Node &Node) {
+Expected<Argument> YAMLRemarkParser::parseArg(yaml::Node &Node) {
   auto *ArgMap = dyn_cast<yaml::MappingNode>(&Node);
   if (!ArgMap)
-    return make_error<YAMLParseError>("expected a value of mapping type.",
-                                      Node);
+    return error("expected a value of mapping type.", Node);
 
-  StringRef KeyStr;
-  StringRef ValueStr;
+  Optional<StringRef> KeyStr;
+  Optional<StringRef> ValueStr;
   Optional<RemarkLocation> Loc;
 
-  for (yaml::KeyValueNode &ArgEntry : *ArgMap)
-    if (Error E = parseArgEntry(ArgEntry, KeyStr, ValueStr, Loc))
-      return E;
-
-  if (KeyStr.empty())
-    return make_error<YAMLParseError>("argument key is missing.", *ArgMap);
-  if (ValueStr.empty())
-    return make_error<YAMLParseError>("argument value is missing.", *ArgMap);
+  for (yaml::KeyValueNode &ArgEntry : *ArgMap) {
+    Expected<StringRef> MaybeKey = parseKey(ArgEntry);
+    if (!MaybeKey)
+      return MaybeKey.takeError();
+    StringRef KeyName = *MaybeKey;
+
+    // Try to parse debug locs.
+    if (KeyName == "DebugLoc") {
+      // Can't have multiple DebugLoc entries per argument.
+      if (Loc)
+        return error("only one DebugLoc entry is allowed per argument.",
+                     ArgEntry);
+
+      if (Expected<RemarkLocation> MaybeLoc = parseDebugLoc(ArgEntry)) {
+        Loc = *MaybeLoc;
+        continue;
+      } else
+        return MaybeLoc.takeError();
+    }
 
-  Args.push_back(Argument{KeyStr, ValueStr, Loc});
+    // If we already have a string, error out.
+    if (ValueStr)
+      return error("only one string entry is allowed per argument.", ArgEntry);
 
-  return Error::success();
-}
+    // Try to parse the value.
+    if (Expected<StringRef> MaybeStr = parseStr(ArgEntry))
+      ValueStr = *MaybeStr;
+    else
+      return MaybeStr.takeError();
 
-Error YAMLRemarkParser::parseArgEntry(yaml::KeyValueNode &ArgEntry,
-                                      StringRef &KeyStr, StringRef &ValueStr,
-                                      Optional<RemarkLocation> &Loc) {
-  StringRef KeyName;
-  if (Error E = parseKey(KeyName, ArgEntry))
-    return E;
-
-  // Try to parse debug locs.
-  if (KeyName == "DebugLoc") {
-    // Can't have multiple DebugLoc entries per argument.
-    if (Loc)
-      return make_error<YAMLParseError>(
-          "only one DebugLoc entry is allowed per argument.", ArgEntry);
-
-    if (Error E = parseDebugLoc(Loc, ArgEntry))
-      return E;
-    return Error::success();
+    // Keep the key from the string.
+    KeyStr = KeyName;
   }
 
-  // If we already have a string, error out.
-  if (!ValueStr.empty())
-    return make_error<YAMLParseError>(
-        "only one string entry is allowed per argument.", ArgEntry);
+  if (!KeyStr)
+    return error("argument key is missing.", *ArgMap);
+  if (!ValueStr)
+    return error("argument value is missing.", *ArgMap);
 
-  // Try to parse a string.
-  if (Error E = parseStr(ValueStr, ArgEntry))
-    return E;
-
-  // Keep the key from the string.
-  KeyStr = KeyName;
-  return Error::success();
+  return Argument{*KeyStr, *ValueStr, Loc};
 }
 
-Error YAMLRemarkParser::parseYAMLElement(yaml::Document &Remark) {
-  // Parsing a new remark, clear the previous one by re-constructing the state
-  // in-place in the Optional.
-  State.emplace(TmpArgs);
+Expected<std::unique_ptr<Remark>> YAMLRemarkParser::next() {
+  if (YAMLIt == Stream.end())
+    return make_error<EndOfFileError>();
 
-  yaml::Node *YAMLRoot = Remark.getRoot();
-  if (!YAMLRoot)
-    return createStringError(std::make_error_code(std::errc::invalid_argument),
-                             "not a valid YAML file.");
-
-  auto *Root = dyn_cast<yaml::MappingNode>(YAMLRoot);
-  if (!Root)
-    return make_error<YAMLParseError>("document root is not of mapping type.",
-                                      *YAMLRoot);
-
-  if (Error E = parseType(State->TheRemark.RemarkType, *Root))
-    return E;
-
-  for (yaml::KeyValueNode &RemarkField : *Root)
-    if (Error E = parseRemarkField(RemarkField))
-      return E;
+  Expected<std::unique_ptr<Remark>> MaybeResult = parseRemark(*YAMLIt);
+  if (!MaybeResult) {
+    // Avoid garbage input, set the iterator to the end.
+    YAMLIt = Stream.end();
+    return MaybeResult.takeError();
+  }
 
-  // If the YAML parsing failed, don't even continue parsing. We might
-  // encounter malformed YAML.
-  if (Stream.failed())
-    return make_error<YAMLParseError>("YAML parsing failed.",
-                                      *Remark.getRoot());
+  ++YAMLIt;
 
-  // Check if any of the mandatory fields are missing.
-  if (State->TheRemark.RemarkType == Type::Unknown ||
-      State->TheRemark.PassName.empty() ||
-      State->TheRemark.RemarkName.empty() ||
-      State->TheRemark.FunctionName.empty())
-    return make_error<YAMLParseError>("Type, Pass, Name or Function missing.",
-                                      *Remark.getRoot());
-
-  return Error::success();
-}
-
-/// Handle a diagnostic from the YAML stream. Records the error in the
-/// YAMLRemarkParser class.
-void YAMLRemarkParser::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);
+  return std::move(*MaybeResult);
 }

lib/Remarks/YAMLRemarkParser.h

@@ -13,7 +13,6 @@
 #ifndef LLVM_REMARKS_YAML_REMARK_PARSER_H
 #define LLVM_REMARKS_YAML_REMARK_PARSER_H
 
-#include "RemarkParserImpl.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Remarks/Remark.h"
@@ -27,112 +26,69 @@
 
 namespace llvm {
 namespace remarks {
-/// Parses and holds the state of the latest parsed remark.
-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;
-  /// Temporary parsing buffer for the arguments.
-  SmallVector<Argument, 8> TmpArgs;
-  /// The string table used for parsing strings.
-  Optional<const ParsedStringTable *> StrTab;
-  /// 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.
-    /// The parser itself is owning this buffer in order to reduce the number of
-    /// allocations.
-    SmallVectorImpl<Argument> &Args;
-    Remark TheRemark;
-
-    ParseState(SmallVectorImpl<Argument> &Args) : Args(Args) {}
-    /// Use Args only as a **temporary** buffer.
-    ~ParseState() { Args.clear(); }
-  };
-
-  /// The current state of the parser. If the parsing didn't start yet, it will
-  /// not be containing any value.
-  Optional<ParseState> State;
-
-  YAMLRemarkParser(StringRef Buf,
-                   Optional<const ParsedStringTable *> StrTab = None)
-      : SM(), Stream(Buf, SM), ErrorString(), ErrorStream(ErrorString),
-        TmpArgs(), StrTab(StrTab) {
-    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.
-  template <typename T> Error parseStr(T &Result, yaml::KeyValueNode &Node);
-  /// Parse one value to an unsigned.
-  template <typename T>
-  Error parseUnsigned(T &Result, yaml::KeyValueNode &Node);
-  /// Parse the type of a remark to an enum type.
-  Error parseType(Type &Result, yaml::MappingNode &Node);
-  /// Parse a debug location.
-  Error parseDebugLoc(Optional<RemarkLocation> &Result,
-                      yaml::KeyValueNode &Node);
-  /// Parse a remark field and update the parsing state.
-  Error parseRemarkField(yaml::KeyValueNode &RemarkField);
-  /// Parse an argument.
-  Error parseArg(SmallVectorImpl<Argument> &TmpArgs, yaml::Node &Node);
-  /// Parse an entry from the contents of an argument.
-  Error parseArgEntry(yaml::KeyValueNode &ArgEntry, StringRef &KeyStr,
-                      StringRef &ValueStr, Optional<RemarkLocation> &Loc);
-
-  /// Handle a diagnostic from the YAML stream. Records the error in the
-  /// YAMLRemarkParser class.
-  static void HandleDiagnostic(const SMDiagnostic &Diag, void *Ctx);
-};
 
 class YAMLParseError : public ErrorInfo<YAMLParseError> {
 public:
   static char ID;
 
-  YAMLParseError(StringRef Message, yaml::Node &Node)
-      : Message(Message), Node(Node) {}
+  YAMLParseError(StringRef Message, SourceMgr &SM, yaml::Stream &Stream,
+                 yaml::Node &Node);
+
+  YAMLParseError(StringRef Message) : Message(Message) {}
 
   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;
+  std::string Message;
 };
 
 /// Regular YAML to Remark parser.
-struct YAMLParserImpl : public ParserImpl {
-  /// The object parsing the YAML.
-  YAMLRemarkParser YAMLParser;
+struct YAMLRemarkParser : public Parser {
+  /// The string table used for parsing strings.
+  Optional<const ParsedStringTable *> StrTab;
+  /// Last error message that can come from the YAML parser diagnostics.
+  /// We need this for catching errors in the constructor.
+  std::string LastErrorMessage;
+  /// Source manager for better error messages.
+  SourceMgr SM;
+  /// Stream for yaml parsing.
+  yaml::Stream Stream;
   /// Iterator in the YAML stream.
   yaml::document_iterator YAMLIt;
-  /// Set to `true` if we had any errors during parsing.
-  bool HasErrors = false;
 
-  YAMLParserImpl(StringRef Buf,
-                 Optional<const ParsedStringTable *> StrTab = None)
-      : ParserImpl{Format::YAML}, YAMLParser(Buf, StrTab),
-        YAMLIt(YAMLParser.Stream.begin()), HasErrors(false) {}
+  YAMLRemarkParser(StringRef Buf,
+                   Optional<const ParsedStringTable *> StrTab = None);
+
+  Expected<std::unique_ptr<Remark>> next() override;
 
-  static bool classof(const ParserImpl *PI) {
-    return PI->ParserFormat == Format::YAML;
+  static bool classof(const Parser *P) {
+    return P->ParserFormat == Format::YAML;
   }
+
+private:
+  /// Create a YAMLParseError error from an existing error generated by the YAML
+  /// parser.
+  /// If there is no error, this returns Success.
+  Error error();
+  /// Create a YAMLParseError error referencing a specific node.
+  Error error(StringRef Message, yaml::Node &Node);
+  /// Parse a YAML remark to a remarks::Remark object.
+  Expected<std::unique_ptr<Remark>> parseRemark(yaml::Document &Remark);
+  /// Parse the type of a remark to an enum type.
+  Expected<Type> parseType(yaml::MappingNode &Node);
+  /// Parse one key to a string.
+  Expected<StringRef> parseKey(yaml::KeyValueNode &Node);
+  /// Parse one value to a string.
+  Expected<StringRef> parseStr(yaml::KeyValueNode &Node);
+  /// Parse one value to an unsigned.
+  Expected<unsigned> parseUnsigned(yaml::KeyValueNode &Node);
+  /// Parse a debug location.
+  Expected<RemarkLocation> parseDebugLoc(yaml::KeyValueNode &Node);
+  /// Parse an argument.
+  Expected<Argument> parseArg(yaml::Node &Node);
 };
 } // end namespace remarks
 } // end namespace llvm

tools/llvm-opt-report/OptReport.cpp

@@ -150,20 +150,32 @@ static bool readLocationInfo(LocationInfoTy &LocationInfo) {
     return false;
   }
 
-  remarks::Parser Parser(remarks::Format::YAML, (*Buf)->getBuffer());
+  Expected<std::unique_ptr<remarks::Parser>> MaybeParser =
+      remarks::createRemarkParser(remarks::Format::YAML, (*Buf)->getBuffer());
+  if (!MaybeParser) {
+    handleAllErrors(MaybeParser.takeError(), [&](const ErrorInfoBase &PE) {
+      PE.log(WithColor::error());
+    });
+    return false;
+  }
+  remarks::Parser &Parser = **MaybeParser;
 
   while (true) {
-    Expected<const remarks::Remark *> RemarkOrErr = Parser.getNext();
-    if (!RemarkOrErr) {
-      handleAllErrors(RemarkOrErr.takeError(), [&](const ErrorInfoBase &PE) {
+    Expected<std::unique_ptr<remarks::Remark>> MaybeRemark = Parser.next();
+    if (!MaybeRemark) {
+      Error E = MaybeRemark.takeError();
+      if (E.isA<remarks::EndOfFileError>()) {
+        // EOF.
+        consumeError(std::move(E));
+        break;
+      }
+      handleAllErrors(MaybeRemark.takeError(), [&](const ErrorInfoBase &PE) {
         PE.log(WithColor::error());
       });
       return false;
     }
-    if (!*RemarkOrErr) // End of file.
-      break;
 
-    const remarks::Remark &Remark = **RemarkOrErr;
+    const remarks::Remark &Remark = **MaybeRemark;
 
     bool Transformed = Remark.RemarkType == remarks::Type::Passed;
 

tools/remarks-shlib/Remarks.exports

@@ -6,6 +6,7 @@ LLVMRemarkDebugLocGetSourceColumn
 LLVMRemarkArgGetKey
 LLVMRemarkArgGetValue
 LLVMRemarkArgGetDebugLoc
+LLVMRemarkEntryDispose
 LLVMRemarkEntryGetType
 LLVMRemarkEntryGetPassName
 LLVMRemarkEntryGetRemarkName

unittests/Remarks/YAMLRemarksParsingTest.cpp

@@ -14,20 +14,31 @@
 using namespace llvm;
 
 template <size_t N> void parseGood(const char (&Buf)[N]) {
-  remarks::Parser Parser(remarks::Format::YAML, {Buf, N - 1});
-  Expected<const remarks::Remark *> Remark = Parser.getNext();
+  Expected<std::unique_ptr<remarks::Parser>> MaybeParser =
+      remarks::createRemarkParser(remarks::Format::YAML, {Buf, N - 1});
+  EXPECT_FALSE(errorToBool(MaybeParser.takeError()));
+  EXPECT_TRUE(*MaybeParser != nullptr);
+
+  remarks::Parser &Parser = **MaybeParser;
+  Expected<std::unique_ptr<remarks::Remark>> Remark = Parser.next();
   EXPECT_FALSE(errorToBool(Remark.takeError())); // Check for parsing errors.
   EXPECT_TRUE(*Remark != nullptr);               // At least one remark.
-  Remark = Parser.getNext();
-  EXPECT_FALSE(errorToBool(Remark.takeError())); // Check for parsing errors.
-  EXPECT_TRUE(*Remark == nullptr); // Check that there are no more remarks.
+  Remark = Parser.next();
+  Error E = Remark.takeError();
+  EXPECT_TRUE(E.isA<remarks::EndOfFileError>());
+  EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors.
 }
 
 template <size_t N>
 bool parseExpectError(const char (&Buf)[N], const char *Error) {
-  remarks::Parser Parser(remarks::Format::YAML, {Buf, N - 1});
-  Expected<const remarks::Remark *> Remark = Parser.getNext();
-  EXPECT_FALSE(Remark); // Expect an error here.
+  Expected<std::unique_ptr<remarks::Parser>> MaybeParser =
+      remarks::createRemarkParser(remarks::Format::YAML, {Buf, N - 1});
+  EXPECT_FALSE(errorToBool(MaybeParser.takeError()));
+  EXPECT_TRUE(*MaybeParser != nullptr);
+
+  remarks::Parser &Parser = **MaybeParser;
+  Expected<std::unique_ptr<remarks::Remark>> Remark = Parser.next();
+  EXPECT_FALSE(Remark); // Check for parsing errors.
 
   std::string ErrorStr;
   raw_string_ostream Stream(ErrorStr);
@@ -42,7 +53,7 @@ TEST(YAMLRemarks, ParsingEmpty) {
 
 TEST(YAMLRemarks, ParsingNotYAML) {
   EXPECT_TRUE(
-      parseExpectError("\x01\x02\x03\x04\x05\x06", "not a valid YAML file."));
+      parseExpectError("\x01\x02\x03\x04\x05\x06", "Got empty plain scalar"));
 }
 
 TEST(YAMLRemarks, ParsingGood) {
@@ -309,17 +320,6 @@ TEST(YAMLRemarks, ParsingWrongArgs) {
                    "",
                    "only one string entry is allowed per argument."));
   // No arg value.
-  EXPECT_TRUE(parseExpectError("\n"
-                   "--- !Missed\n"
-                   "Pass: inline\n"
-                   "Name: NoDefinition\n"
-                   "Function: foo\n"
-                   "Args:\n"
-                   "  - Callee: ''\n"
-                   "  - DebugLoc: { File: a, Line: 1, Column: 2 }\n"
-                   "",
-                   "argument value is missing."));
-  // No arg value.
   EXPECT_TRUE(parseExpectError("\n"
                    "--- !Missed\n"
                    "Pass: inline\n"
@@ -354,12 +354,18 @@ TEST(YAMLRemarks, Contents) {
                   "  - String: ' because its definition is unavailable'\n"
                   "\n";
 
-  remarks::Parser Parser(remarks::Format::YAML, Buf);
-  Expected<const remarks::Remark *> RemarkOrErr = Parser.getNext();
-  EXPECT_FALSE(errorToBool(RemarkOrErr.takeError()));
-  EXPECT_TRUE(*RemarkOrErr != nullptr);
+  Expected<std::unique_ptr<remarks::Parser>> MaybeParser =
+      remarks::createRemarkParser(remarks::Format::YAML, Buf);
+  EXPECT_FALSE(errorToBool(MaybeParser.takeError()));
+  EXPECT_TRUE(*MaybeParser != nullptr);
+
+  remarks::Parser &Parser = **MaybeParser;
+  Expected<std::unique_ptr<remarks::Remark>> MaybeRemark = Parser.next();
+  EXPECT_FALSE(
+      errorToBool(MaybeRemark.takeError())); // Check for parsing errors.
+  EXPECT_TRUE(*MaybeRemark != nullptr);      // At least one remark.
 
-  const remarks::Remark &Remark = **RemarkOrErr;
+  const remarks::Remark &Remark = **MaybeRemark;
   EXPECT_EQ(Remark.RemarkType, remarks::Type::Missed);
   EXPECT_EQ(checkStr(Remark.PassName, 6), "inline");
   EXPECT_EQ(checkStr(Remark.RemarkName, 12), "NoDefinition");
@@ -408,9 +414,10 @@ TEST(YAMLRemarks, Contents) {
     ++ArgID;
   }
 
-  RemarkOrErr = Parser.getNext();
-  EXPECT_FALSE(errorToBool(RemarkOrErr.takeError()));
-  EXPECT_EQ(*RemarkOrErr, nullptr);
+  MaybeRemark = Parser.next();
+  Error E = MaybeRemark.takeError();
+  EXPECT_TRUE(E.isA<remarks::EndOfFileError>());
+  EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors.
 }
 
 static inline StringRef checkStr(LLVMRemarkStringRef Str,
@@ -487,6 +494,8 @@ TEST(YAMLRemarks, ContentsCAPI) {
     ++ArgID;
   } while ((Arg = LLVMRemarkEntryGetNextArg(Arg, Remark)));
 
+  LLVMRemarkEntryDispose(Remark);
+
   EXPECT_EQ(LLVMRemarkParserGetNext(Parser), nullptr);
 
   EXPECT_FALSE(LLVMRemarkParserHasError(Parser));
@@ -516,12 +525,18 @@ TEST(YAMLRemarks, ContentsStrTab) {
                 115);
 
   remarks::ParsedStringTable StrTab(StrTabBuf);
-  remarks::Parser Parser(remarks::Format::YAML, Buf, StrTab);
-  Expected<const remarks::Remark *> RemarkOrErr = Parser.getNext();
-  EXPECT_FALSE(errorToBool(RemarkOrErr.takeError()));
-  EXPECT_TRUE(*RemarkOrErr != nullptr);
-
-  const remarks::Remark &Remark = **RemarkOrErr;
+  Expected<std::unique_ptr<remarks::Parser>> MaybeParser =
+      remarks::createRemarkParser(remarks::Format::YAML, Buf, &StrTab);
+  EXPECT_FALSE(errorToBool(MaybeParser.takeError()));
+  EXPECT_TRUE(*MaybeParser != nullptr);
+
+  remarks::Parser &Parser = **MaybeParser;
+  Expected<std::unique_ptr<remarks::Remark>> MaybeRemark = Parser.next();
+  EXPECT_FALSE(
+      errorToBool(MaybeRemark.takeError())); // Check for parsing errors.
+  EXPECT_TRUE(*MaybeRemark != nullptr);      // At least one remark.
+
+  const remarks::Remark &Remark = **MaybeRemark;
   EXPECT_EQ(Remark.RemarkType, remarks::Type::Missed);
   EXPECT_EQ(checkStr(Remark.PassName, 6), "inline");
   EXPECT_EQ(checkStr(Remark.RemarkName, 12), "NoDefinition");
@@ -570,9 +585,10 @@ TEST(YAMLRemarks, ContentsStrTab) {
     ++ArgID;
   }
 
-  RemarkOrErr = Parser.getNext();
-  EXPECT_FALSE(errorToBool(RemarkOrErr.takeError()));
-  EXPECT_EQ(*RemarkOrErr, nullptr);
+  MaybeRemark = Parser.next();
+  Error E = MaybeRemark.takeError();
+  EXPECT_TRUE(E.isA<remarks::EndOfFileError>());
+  EXPECT_TRUE(errorToBool(std::move(E))); // Check for parsing errors.
 }
 
 TEST(YAMLRemarks, ParsingBadStringTableIndex) {
@@ -584,13 +600,18 @@ TEST(YAMLRemarks, ParsingBadStringTableIndex) {
   StringRef StrTabBuf = StringRef("inline");
 
   remarks::ParsedStringTable StrTab(StrTabBuf);
-  remarks::Parser Parser(remarks::Format::YAML, Buf, StrTab);
-  Expected<const remarks::Remark *> Remark = Parser.getNext();
-  EXPECT_FALSE(Remark); // Expect an error here.
+  Expected<std::unique_ptr<remarks::Parser>> MaybeParser =
+      remarks::createRemarkParser(remarks::Format::YAML, Buf, &StrTab);
+  EXPECT_FALSE(errorToBool(MaybeParser.takeError()));
+  EXPECT_TRUE(*MaybeParser != nullptr);
+
+  remarks::Parser &Parser = **MaybeParser;
+  Expected<std::unique_ptr<remarks::Remark>> MaybeRemark = Parser.next();
+  EXPECT_FALSE(MaybeRemark); // Expect an error here.
 
   std::string ErrorStr;
   raw_string_ostream Stream(ErrorStr);
-  handleAllErrors(Remark.takeError(),
+  handleAllErrors(MaybeRemark.takeError(),
                   [&](const ErrorInfoBase &EIB) { EIB.log(Stream); });
   EXPECT_TRUE(
       StringRef(Stream.str())