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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Some inline comments, overall this looks good but I'll need to do another pass to make sure I understand everything correctly :-)



================
Comment at: llvm/include/llvm/Remarks/Remark.h:65
+  /// The type of the remark.
+  enum Type Type = Type::Unknown;
+
----------------
This looks a little odd, any chance you could give the variable a different name?


================
Comment at: llvm/include/llvm/Remarks/RemarkParser.h:36
+  /// Create a parser parsing \p Buffer to Remark objects.
+  /// This enforces that this constructor is only used for YAML remarks.
+  Parser(StringRef Buffer, YAMLType);
----------------
I don't fully understand how this enforces anything?


================
Comment at: llvm/lib/Remarks/RemarkParser.cpp:86
+      return wrap(Remark);
+    // else we reached the end.
+  } else {
----------------
early return?


================
Comment at: llvm/lib/Remarks/YAMLRemarkParser.cpp:130
+  for (yaml::KeyValueNode &ArgEntry : *ArgMap) {
+    StringRef KeyName;
+    if (Error E = parseKey(KeyName, ArgEntry))
----------------
Can we extract a function from the loop body?


================
Comment at: llvm/lib/Remarks/YAMLRemarkParser.cpp:182
+  for (yaml::KeyValueNode &RemarkField : *Root) {
+    StringRef KeyName;
+    if (Error E = parseKey(KeyName, RemarkField))
----------------
Same here, can we simplify this by extracting a function?


================
Comment at: llvm/lib/Remarks/YAMLRemarkParser.h:46
+    /// Temporary parsing buffer for the arguments.
+    SmallVectorImpl<Argument> *Args;
+    Remark Remark;
----------------
Does this have to be a pointer? Can it be a reference?

One a more general level: why does the ParseState not own the buffer? Are you reducing the number of allocations? Maybe worth adding a comment with the reason why.


================
Comment at: llvm/lib/Remarks/YAMLRemarkParser.h:114
+  /// Iterator in the YAML stream.
+  yaml::document_iterator DI;
+  /// Set to `true` if we had any errors during parsing.
----------------
`DI` outside the context of this class is really hard to follow. Consider adding a getter or giving this a more meaningful name.


================
Comment at: llvm/lib/Remarks/YAMLRemarkParser.h:116
+  /// Set to `true` if we had any errors during parsing.
+  bool HadAnyErrors = false;
+
----------------
nit: had or has?


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

https://reviews.llvm.org/D59049