aadsm planned changes to this revision.
aadsm marked 2 inline comments as done.
aadsm added inline comments.
================
Comment at: lldb/source/Utility/Timer.cpp:110-123
+namespace {
+struct Stats {
+ uint64_t nanos;
+ uint64_t nanos_total;
+ uint64_t nanos_child;
+ uint64_t count;
+};
----------------
labath wrote:
> You can just move the `const char *` into the `Stats` struct and get rid of
> the std::pair. Then the `CategoryMapIteratorSortCriterion` thingy can also
> become a regular operator<.
ah yeah, that will be much better.
================
Comment at: lldb/unittests/Utility/TimerTest.cpp:89
+ // 0.102982273 sec (total: 0.126s; child: 0.023s; count: 1) for CAT1
+ // 0.023058228 sec (total: 0.036s; child: 0.012s; count: 2) for CAT2
+ StreamString ss;
----------------
labath wrote:
> I'm not sure this second line is really helpful here. Looking at this output,
> I would have never guessed that these numbers mean we were running two
> recursive timers belonging to the same category, and the entire thing
> finished in 20ms.
>
> Maybe this isn't a real usage problem, as normally we don't have recursive
> timers (I believe), but in that case, we probably shouldn't have them in this
> test either (as right now it is the only documentation about how these times
> are supposed to work).
>
> What's the reason for using recursive timers here? If you just wanted to
> check that the invocation count is 2, then you can wrap the two timers in
> `{}` blocks, so that they execute "sequentially" instead of recursively.
> (Which I guess would match the typical scenario where a timer-enabled
> function calls another timer-enabled function two times in a loop.)
you're totally right, I actually didn't think much about it when I wrote the
test but this should definitely be sequential not recursive.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61235/new/
https://reviews.llvm.org/D61235
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
