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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: include/llvm-c/OptRemarks.h:84
+typedef struct {
+  // e.g. Missed, Passed
+  LLVMOptRemarkStringRef RemarkType;
----------------
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?


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


================
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.
----------------
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)
```


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:303
+    error("YAML parsing failed.", *Remark.getRoot());
+    return true;
+  }
----------------
JDevlieghere wrote:
> 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.
Fair point, thanks! It seems that the [[ http://lists.llvm.org/pipermail/llvm-dev/2018-September/126178.html | discussion ]] was going more towards returning true on success. I'll update everything accordingly.


================
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.
----------------
paquette wrote:
> Skip shouldn't be capitalized
Actually, the comment is irrelevant now. Removed.


================
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;
----------------
paquette wrote:
> Should this comment be above `Tag`?
Tag was too confusing. I removed it and called it Type.


================
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;
----------------
paquette wrote:
> I think that the first letter in these error messages should be capitalized
I would like to, but they show up like this in the diagnostics:

```
error: YAML:2:1: error: key is not a string.

{ a: b }:            inline
^
```

and clang follows this as well.


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:252
+
+  return !OldSkipThisOne;
+}
----------------
paquette wrote:
> Why are we returning `!OldSkipThisOne`? A comment would be good here.
Removed as part of the error refactoring.


https://reviews.llvm.org/D52776