[PATCH] D54201: [XRay] Improve FDR trace handling and error messaging

[Marcus Boerger via Phabricator via llvm-commits]

mboerger added inline comments.


================
Comment at: llvm/include/llvm/XRay/FDRRecords.h:129
+
+  static bool classof(const Record *R) {
+    return R->getRecordType() == RecordKind::RK_Metadata_BufferExtents;
----------------
This is a weird interface. You basically want to associate a type enum with a class type. But here you do it in a kind of inverse way. Since you don't really use it, I don't know what better to do though.


================
Comment at: llvm/lib/XRay/FDRRecordProducer.cpp:80
+FileBasedRecordProducer::findNextBufferExtent() {
+  // We keep seeing for a specific kind of record from the data extractor, one
+  // byte at a time, one which resembles a buffer extents record.
----------------
Please reword this


================
Comment at: llvm/lib/XRay/FDRRecordProducer.cpp:84
+  while (!R) {
+    auto PreReadOffset = OffsetPtr;
+    uint8_t FirstByte = E.getU8(&OffsetPtr);
----------------
one would expect the external 'OffsetPtr' to stay unaffected and instead of 'PreReadOffset' having a 'CurrentOffset' or so variable. Please clarify or change.


================
Comment at: llvm/lib/XRay/FDRRecordProducer.cpp:91
+
+    if (FirstByte & 0x01u) {
+      auto LoadedType = FirstByte >> 1;
----------------
comment what this check does, or even better provide a static helper function that is used everywhere you do this


================
Comment at: llvm/lib/XRay/FDRRecords.cpp:34
+  switch (K) {
+  case RecordKind::RK_Metadata:
+    return "Metadata";
----------------
this screams for using a macro :-)


================
Comment at: llvm/lib/XRay/FDRTraceWriter.cpp:46
 Error writeMetadata(support::endian::Writer &OS, Values &&... Ds) {
-  uint8_t FirstByte = (Kind << 1) | uint8_t{0x01};
+  uint8_t FirstByte = (Kind << 1) | uint8_t{0x01u};
   auto T = std::make_tuple(std::forward<Values>(std::move(Ds))...);
----------------
so this should probably also get a helper function


https://reviews.llvm.org/D54201