[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;
----------------
thegameg wrote:
> JDevlieghere wrote:
> > 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. 
> The grouping is mostly mandatory vs optional. Are you suggesting something else?
No, it's just that it wasn't clear to me. Maybe remove the newlines between DebugLoc and Hotness, and Hotness and Args, to mirror the grouping for the mandatory fields.


================
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.
----------------
thegameg wrote:
> JDevlieghere wrote:
> > 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. 
> Mostly for consistency with `parseDebugLoc` and `parseArg`. Another nice thing is that I can write things like:
> 
> ```
> parseValue(Str, Node) && errorAndSkip("error message here", Node)
> ```
My comment was more general now that you're using Error you could return an Expected. Anyway, it's probably not worth is you'd need an extra struct for `parseDebugLoc` and you'd loose the nice thing you mentioned. 


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:119
+
+static LLVMOptRemarkStringRef convert(StringRef Str) {
+  return {Str.data(), static_cast<uint32_t>(Str.size())};
----------------
How about something less generic like `toOptRemarkStr`? 


https://reviews.llvm.org/D52776