[PATCH] D34339: [XRay] fix and clarify comments in the log file decoder [NFC]

Martin Pelikán via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 19 18:39:20 PDT 2017


pelikan added inline comments.


================
Comment at: lib/XRay/Trace.cpp:10-11
 //
-// XRay log reader implementation.
+// XRay log reader implementation.  Decodes logs written using compiler-rt's
+// naive and Flight Data Recorder XRay logging formats.
 //
----------------
dberris wrote:
> This technically isn't strictly true. While there is an implementation of the naive and FDR mode logging, those may not be written by the implementation in compiler-rt. Making it sound like there's an actual dependency (as opposed to a coincidental dependency) may be misleading.
> 
> For instance, it's possible to write logs in this format without using the compiler-rt implementation.
> 
> Working on documenting the log details/format as opposed to referring to the compiler-rt implementation would be a strictly better approach. I'd rather change this comment to say the explicit versions of the log we're supporting and not mentioning anything in compiler-rt.
May not be.  But currently (and for the forseeable future) will be.  Currently, a person will see two separate and *very* different implementations, and not know if they're supposed to be related or not just because the data format appears to coincide.

The point of this is obviously not documenting the format.  It is supposed to tell the reader where an example of the counterpart can be found.  I've updated it so it reflects the fact this doesn't have to be a hard dependency.

When/if there is a formal spec to the format, I'll be happy to reword this.  (IIUC this isn't a priority for anyone yet, because there are only these two instances so far.)


https://reviews.llvm.org/D34339





More information about the llvm-commits mailing list