jj10306 added inline comments.
================
Comment at: lldb/include/lldb/Target/TraceCursor.h:265
+class TraceEventsUtils {
+public:
----------------
nit: maybe this will change in the future to where this will have data and
instance members, but if not, consider just using a namespace here to house the
utility functions since the class isn't really providing any state?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:75
+
+ /// \return \b true iff this struct holds a libipt error.
+ explicit operator bool() const;
----------------
wallace wrote:
> jj10306 wrote:
> >
> oh, i wanted to mean if and only if. Or is that too mathematical and less
> colloquial?
Lol my first thought actually was "I wonder if he meant if and only if, so this
isn't a typo".
Up to you if you want to keep if or leave it as iff
================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258
/// first one. Otherwise, this map will be empty.
std::map<size_t, uint64_t> m_instruction_timestamps;
/// This is the chronologically last TSC that has been added.
----------------
wallace wrote:
> jj10306 wrote:
> > jj10306 wrote:
> > > I know this isn't related to these changes, but this should be updated to
> > > be consistent with the other `instruction_index -> XXX` maps in this
> > > class.
> >
> in this case we can't do that, because when doing random accesses in the
> trace, we need to quickly find the most recent TSC for the new instruction,
> which is done with binary search using a map. This is impossible in a hash
> map like DenseMap
I know this is a pedantic discussion, but isn't lookup in hash tables amortized
O(1) (assuming a good hash that doesn't take too long to execute or produce the
same hash for different values) whereas in an ordered map it's O(logn)?
IIUC, you should only use an ordered map when you rely on the order property of
it.
In any case, we should use `uint64_t` here instead of `size_t` to be consistent
with the type for instruction index used in the other maps.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123982/new/
https://reviews.llvm.org/D123982
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
