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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: include/llvm-c/OptRemarks.h:84
+typedef struct {
+  // e.g. Missed, Passed
+  LLVMOptRemarkStringRef RemarkType;
----------------
The inconsistency in the comments bothers me. Sometimes the key is mentioned, followed by a colon or a period, sometimes it's the value, possibly between single quotes. How about putting both between quotes? The grouping is also not clear to me. 


================
Comment at: include/llvm-c/OptRemarks.h:124
+ * The value pointed by the return value is invalidated by the next call to
+ * LLVMOptRemarkParserGetNext().
+ *
----------------
What happens if you call this after you encounter an error? 


================
Comment at: include/llvm-c/OptRemarks.h:146
+ *
+ * Here is a quick example on the usage:
+ *
----------------
s/on/of/


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:24
+struct RemarkParser {
+  // Source manager for better error messages.
+  SourceMgr SM;
----------------
I usually use doxygen style comments even in lib, in case it ever gets moved to a header.


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:78
+  // Parse one value to a string.
+  bool parseValue(StringRef &Result, yaml::KeyValueNode &Node);
+  // Parse one value to an unsigned.
----------------
Why don't you return the StringRef? Can the function return false and a non-empty string / valid optional? If so that'd be a good thing to mention here. Same for the parse functions below. 


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:143
+  // the debug loc.
+  bool oldSkipThisOne = State.SkipThisOne;
+  State.SkipThisOne = false;
----------------
s/old/Old/, same in the functions below.


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:239
+    Args.push_back(LLVMOptRemarkArg{
+        LLVMOptRemarkStringRef{KeyStr.data(),
+                               static_cast<uint32_t>(KeyStr.size())},
----------------
Maybe extract a convenience method for constructing a LLVMOptRemarkStringRef from a StringRef?


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:303
+    error("YAML parsing failed.", *Remark.getRoot());
+    return true;
+  }
----------------
So `true` means there was an error and `false` means success? That's probably worth mentioning in a comment as well. There was a discussion about this on the mailing list, but I'm not sure if anything was decided yet.


https://reviews.llvm.org/D52776