[PATCH] D51289: [XRay] FDR Record Producer/Consumer Implementation

[Dean Michael Berris via Phabricator via llvm-commits]

dberris marked an inline comment as done.
dberris added inline comments.


================
Comment at: llvm/trunk/lib/XRay/FDRRecordProducer.cpp:50
+      MetadataRecordKinds::Pid,
+  };
+  switch (Mapping[T]) {
----------------
dberris wrote:
> RKSimon wrote:
> > @dberris  Why are you mapping an integer to an enum with the same value?!? Replace the uint8_t T arg with an MetadataRecordKinds enum arg, or just get rid of the enum (AFAICT its only used in this function) and use the uint8_t directly.
> So, there are two reasons:
> 
> - The data we're getting is a byte, and we need to decide whether the byte maps to the number we're expecting. The caller of the function (below) only has access to the byte.
> 
> - I could just static cast the byte into a `MetadataRecordKinds` value, but that's undefined behaviour (not all the values of T will be valid values for the enum).
> 
> If I get rid of the enum, I have to keep the enum names and values as comments (previous version of this patch).
> 
> Ultimately the enum is a convenience. This mapping though is a little suspect.
> 
> This is where being able to just share the enum definition between compiler-rt and llvm would make things a lot easier. :(
Actually, I think you're right -- there's a better way to do this. Fixed in r341205.


Repository:
  rL LLVM

https://reviews.llvm.org/D51289