[PATCH] D67134: [Remarks] Add parser for bitstream remarks

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

I really like your code style, useful comments, good abstractions, and great error handling. This was fun to review. A few things inline but overall this LGTM!



================
Comment at: llvm/lib/Remarks/BitstreamRemarkParser.cpp:43
+  BitstreamCursor &Stream = Parser.Stream;
+  // Note: 2 is used here because it's the max number of fields we have per
+  // record.
----------------
Looks like this comment needs to move down one line or `Blob` should be declared after the `SmallVector`?


================
Comment at: llvm/lib/Remarks/BitstreamRemarkParser.cpp:90
+  BitstreamCursor &Stream = Parser.Stream;
+  // Note: 5 is used here because it's the max number of fields we have per
+  // record.
----------------
Same as previous comment.


================
Comment at: llvm/lib/Remarks/BitstreamRemarkParser.cpp:193
+  }
+  // If we're here, it means we didn't get an END_BLOCK yet, but we're at the
+  // end of the stream. In this case, error.
----------------
Is this actually reachable?


================
Comment at: llvm/lib/Remarks/BitstreamRemarkParser.cpp:509
+
+  if (Expected<StringRef> RemarkName = (*StrTab)[*Helper.RemarkNameIdx])
+    R.RemarkName = *RemarkName;
----------------
It's a little unfortunate that this pattern is repeated here but I don't have a good alternative. I considered suggesting putting it behind a macro, but given that there's only three instances that might be overkill and actually hurt readability. 


================
Comment at: llvm/lib/Remarks/BitstreamRemarkParser.h:66
+  /// Helper function.
+  Error processCommonMeta(BitstreamMetaParserHelper &Helper);
+  Error processStandaloneMeta(BitstreamMetaParserHelper &Helper);
----------------
I think these helpers could be private? Doing so would help convey what's part of the interface and what's not. The same applies to the other structs. 


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

https://reviews.llvm.org/D67134