[PATCH] D52776: [OptRemarks] Add library for parsing optimization remarks

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Mostly style nits here.

Biggest thing is wondering if we can use `llvm::Error` here instead of the `error` and `errorAndSkip` functions?



================
Comment at: include/llvm-c/OptRemarks.h:110
+ *
+ * \p Buf has to be != NULL.
+ *
----------------
How about "\p Buf cannot be NULL"?


================
Comment at: include/llvm-c/OptRemarks.h:123
+ *
+ * The value pointed by the return value is invalidated by the next call to
+ * LLVMOptRemarkParserGetNext().
----------------
pointed to by


================
Comment at: include/llvm-c/OptRemarks.h:126
+ *
+ * In the case of the parser reaching the end of the buffer, the return value
+ * will be NULL.
----------------
Simpler:

If the parser reaches the end of the buffer, the return value will be NULL.


================
Comment at: include/llvm-c/OptRemarks.h:131-132
+ *
+ * 1) LLVMOptRemarkParserHasError() will return `1`.
+ * 2) LLVMOptRemarkParserGetErrorMessage() will return a descriptive error
+ * message.
----------------
Space between list items for consistency with following list. e.g

1) x

2) y


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:28
+  yaml::Stream Stream;
+
+  /// Storage for the error stream.
----------------
Remove newline for consistency?


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:34
+  /// Iterator in the YAML stream.
+  yaml::document_iterator di;
+  /// The parsed remark (if any).
----------------
s/di/DI/


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:44
+    SmallVectorImpl<LLVMOptRemarkArg> *Args;
+    /// Mandatory. Will Skip the handler if the remark does not contain them.
+    StringRef Tag; /// The type of the remark. e.g. Passed, Failed, etc.
----------------
Skip shouldn't be capitalized


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:45
+    /// Mandatory. Will Skip the handler if the remark does not contain them.
+    StringRef Tag; /// The type of the remark. e.g. Passed, Failed, etc.
+    StringRef Pass;
----------------
Should this comment be above `Tag`?


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:90
+  /// Report an error to the error stream.
+  bool error(StringRef Report, yaml::Node &N) {
+    Stream.printError(&N, Twine(Report) + Twine('\n'));
----------------
Can we use `llvm::Error` for this and the other error handling stuff here?


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:206
+    auto *Key = dyn_cast<yaml::ScalarNode>(ArgEntry.getKey());
+    if (!Key && errorAndSkip("key is not a string.", ArgEntry))
+      continue;
----------------
I think that the first letter in these error messages should be capitalized


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:252
+
+  return !OldSkipThisOne;
+}
----------------
Why are we returning `!OldSkipThisOne`? A comment would be good here.


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:313
+
+  // Call the handler if we don't need to Skip it.
+  if (!State.SkipThisOne)
----------------
Skip doesn't need to be capitalized


https://reviews.llvm.org/D52776