[PATCH] D29697: [XRAY] [x86_64] Adding a Flight Data filetype reader to the llvm-xray Trace implementation.

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Thanks for the comments Dean. I've updated the diff with some changes, the most substantial of which is breaking out different RecordKind state transitions into individual functions.



================
Comment at: lib/XRay/Trace.cpp:57
+
+Error NaiveLogLoader(StringRef Data, XRayFileHeader &FileHeader,
+                     std::vector<XRayRecord> &Records) {
----------------
dberris wrote:
> s/Naive/naive/
I don't like that these functions are named as nouns instead of verbs either. loadNaiveFormatLog is more to my taste.


================
Comment at: lib/XRay/Trace.cpp:257-258
+          break;
+        case 1: // EXIT
+        case 2: // TAIL_EXIT
+          Record.Type = RecordTypes::EXIT;
----------------
dberris wrote:
> I suspect you could be able to use the enum values as case labels here for the record types (using the conversion to int support for enum classes). It would be great if that works here. :)
Alas, enum classes require static_cast to convert to int, in contrast to humble enums. There's little harm in having case statements with the static_cast though (e.g. case static_cast<uint8_t>(RecordType::ENTER): )

This should work for ENTER/EXIT. TAIL_EXIT is not yet defined in RECORD_TYPES, and I left defining it for a follow up CL since there is code for TAIL exit deduction that will have to be touched.


https://reviews.llvm.org/D29697