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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Remarks/YAMLRemarkParser.h:46
+    /// Temporary parsing buffer for the arguments.
+    SmallVectorImpl<Argument> *Args;
+    Remark Remark;
----------------
JDevlieghere wrote:
> Does this have to be a pointer? Can it be a reference?
> 
> One a more general level: why does the ParseState not own the buffer? Are you reducing the number of allocations? Maybe worth adding a comment with the reason why.
I had issues with assigning a new state, but I just discovered `Optional<T>::emplace`. Thanks! Added the reason for the buffer re-use.


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

https://reviews.llvm.org/D59049