[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/source/Target/TraceHTR.cpp:160
+          current_instruction_load_address);
+      valid_cursor = cursor.Next();
+      if (current_instruction_type &
----------------
jj10306 wrote:
> wallace wrote:
> > valid_cursor is not a nice name, just call it no_more_data, end_of_trace, or something like that
> I was trying to use "positive logic", otherwise using something like `end_of_trace` would require and negating every use of it and every time the `Next()` is called (i.e `end_of_trace =!cursor->Next()`).
> 
> This is obviously a very minor thing, but I think using a well named "positive logic" variable makes the code easier to read - thoughts on using a name like `more_data_in_trace`? It's definitely harder to come up with a well named positive logic variable so let me know if you have any better suggestions 
> 
more_data_in_trace is fine!


================
Comment at: lldb/source/Target/TraceHTR.cpp:245
+      os.flush();
+      if (os.has_error()) {
+        s.Printf("error exporting trace representation to %s\n", outfile_cstr);
----------------
jj10306 wrote:
> wallace wrote:
> > can you also show the actual error message from os?
> I don't believe `llvm::raw_fd_ostream` stores the error message, only the error code.
you can invoke os.error() and then convert that value to a string :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105741/new/

https://reviews.llvm.org/D105741