jj10306 marked 10 inline comments as done.
jj10306 added inline comments.
================
Comment at: lldb/include/lldb/Target/TraceCursor.h:193
+ // TODO: add docs and rename once naming convention is agreed upon
+ virtual llvm::Optional<uint64_t> GetNanos() = 0;
+
----------------
wallace wrote:
> What about just naming it GetTimestamp and return std::chrono::nanoseconds?
> That way we use c++ time conversion libraries and we don't include the time
> granularity in the name
sgtm - do you think the `GetTimestampCounter` API's name should remain the same
or change it to something like `GetTsc`? This file is trace agnostic so tying
it to TSC could be confusing since, afaiu, TSC is intel specific, but I noticed
the docs of that method mention "TSC", so curious to hear your thoughts.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:46
+// TODO: update after naming convention is agreed upon
+class TimestampCounterRate {
+ public:
----------------
wallace wrote:
> This class is generic, so better not enforce the meaning of the zero tsc.
> Each trace plugin might be opinionated regarding this timestamp
Good point. Given that this class is generic, would it make more sense to move
it from this trace specific file `TraceIntelPTGDBRemotePackets.h` to
`TraceGDBRemotePackets.h` since that file is trace agnostic?
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+ public:
----------------
jj10306 wrote:
> wallace wrote:
> > davidca wrote:
> > > nit, Intel documentation and kernel refers this as TSC, not
> > > TimestampCounter, in order to differentiate this awful name from
> > > everywhere else that "Timestamp" and "Counter" is used. Perhaps embrace
> > > TSC to make it easier to google info?
> > Ahh!! Then let's do what David says. Let's call it tsc everywhere
> Which do you think is a better name to replace `TimestampCounterRate` with -
> `TscConversionParams` (as mentioned in the previous comment,
> "tsc_conversion_params" will be the new key in the TraceGetState packet
> schema, so this would be consistent with that) or `TscConversioninfo` or
> `TscConverter` (since the function of the class is to provide functionality
> to convert Tsc to time)?
> @davidca @wallace wdyt?
Bumping this discussion related to naming
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120595/new/
https://reviews.llvm.org/D120595
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
