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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
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.
----------------
JDevlieghere wrote:
> Is this actually reachable?
I was imagining the case where we have something like a bunch of record entries followed by an end of file with no `END_BLOCK`. After the last successfully parsed record, we `continue`, then the loop condition is false, and we end up with an unterminated block because the stream is at the end, so we need to report an error.


================
Comment at: llvm/lib/Remarks/BitstreamRemarkParser.cpp:509
+
+  if (Expected<StringRef> RemarkName = (*StrTab)[*Helper.RemarkNameIdx])
+    R.RemarkName = *RemarkName;
----------------
JDevlieghere wrote:
> 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. 
I agree, I thought of a macro but I came to the same conclusion as you.


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

https://reviews.llvm.org/D67134