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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
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);
----------------
thegameg wrote:
> JDevlieghere wrote:
> > I don't fully understand how this enforces anything?
> Right, it doesn't!
So why is it there? :-) 


================
Comment at: llvm/lib/Remarks/RemarkParser.cpp:86
+      return wrap(Remark);
+    // else we reached the end.
+  } else {
----------------
JDevlieghere wrote:
> early return?
I would've done it the other way around, with early returns for everything that can go wrong first. I think that could save you another level of indentation.


================
Comment at: llvm/lib/Remarks/YAMLRemarkParser.cpp:25
+  auto *Key = dyn_cast<yaml::ScalarNode>(Node.getKey());
+  if (!Key)
+    return make_error<YAMLParseError>("key is not a string.", Node);
----------------
```
if(auto* Key = ....) { 
  Result = Key->getRawValue();
  return Error::success();
}
return make_error<YAMLParseError>("key is not a string.", Node);
```


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

https://reviews.llvm.org/D59049