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

[Keith via Phabricator via llvm-commits]

kpw marked 3 inline comments as done.
kpw added inline comments.


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


================
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)),
----------------
dberris wrote:
> I suspect it's more readable if you just used '+' here. Idiomatic Twine usage seems to prefer concatenation with '+' for readability reasons.
Will a conversion operator be called for literal strings past the first.

Is it idiomatic to write Twine("Concatenating ") + "several" + " strings " + " that " + " become twines " and get a Twine as the result?


https://reviews.llvm.org/D31385