[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:74
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {
+  /// `nullptr` if no tsc conversion rate exists.
+  llvm::Optional<TimestampCounterRateSP> tsc_rate;
----------------
wallace wrote:
> avoid using nullptr when you have an Optional. 
Ideally this would be a plain `TimestampCounterRateSP` variable and nullptr would denote emptyness. Since (shared) pointers already have a natural empty state, it's best to use that. Adding an extra Optional layer just creates ambiguity.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:22
 #include "llvm/ADT/None.h"
+#include <memory>
 
----------------
wallace wrote:
> jj10306 wrote:
> > 
> memory should be defined as a new include block between lines 9 and 11. That's just the pattern we follow
It's actually a pattern we should not follow. When all of the includes are grouped in one block, then clang-format automatically reorders them to conform to the [[https://llvm.org/docs/CodingStandards.html#include-style | llvm coding standards ]].


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

https://reviews.llvm.org/D120595