[PATCH] D31385: [XRay] Update FDR log reader to be aware of buffer sizes per thread.

[Dean Michael Berris via Phabricator via llvm-commits]

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

LGTM with some nits.



================
Comment at: lib/XRay/Trace.cpp:138
+  case FDRState::Token::NEW_BUFFER_RECORD:
+    return Twine("NEW_BUFFER_RECORD");
+  case FDRState::Token::WALLCLOCK_RECORD:
----------------
Just curious -- did you need to actually explicitly create an instance of Twine? Or would just returning the literal work just fine?


================
Comment at: lib/XRay/Trace.cpp:385-386
+      State.Expects = FDRState::Token::NEW_BUFFER_RECORD;
+      continue;
+    } else if (State.Expects == FDRState::Token::SCAN_TO_END_OF_THREAD_BUF) {
+      RecordSize = State.CurrentBufferSize - State.CurrentBufferConsumed;
----------------
I don't think you need the else since you're already continuing in the body of the true branch.


================
Comment at: lib/XRay/Trace.cpp:423-426
+            .concat(fdrStateToTwine(State.Expects))
+            .concat(Twine(". Remaining expected bytes in thread buffer total "))
+            .concat(
+                Twine(State.CurrentBufferSize - State.CurrentBufferConsumed)),
----------------
I suspect it's more readable if you just used '+' here. Idiomatic Twine usage seems to prefer concatenation with '+' for readability reasons.


https://reviews.llvm.org/D31385