labath added a comment.
I don't see myself using this command personally, but I agree that splitting
the "disable stats collection" and "dump accumulated stats" into two actions
seems a better choice (maybe disable could do a final auto-dump though). I also
think the header/footer is not needed for a primarily interactive tool. If we
have a command that dumps so much data that the user cannot tell where the
output starts and ends then we're doing something wrong (PS: I wish we had
automatic pagination for the backtrace command).
I am also wondering whether statistics should be a top-level command, or should
we put it under "log". We already have "log timers enable/disable/...", which
are doing something similar to this, so it seems like a good idea to move these
two things closer together.
================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c:12
+ //%self.expect("stats enable")
+ //%self.expect("frame var", substrs=['27'])
+ //%self.expect("stats disable", substrs=['frame var successes : 1', 'frame
var failures : 0'])
----------------
Maybe test for failure as well (I guess trying to print an non-existing
variable should do the trick)?
================
Comment at: lldb/source/Commands/CommandObjectStats.cpp:31-35
+ if (!target) {
+ result.AppendError("stats command needs a target");
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ }
----------------
I think we have some way (should involve `eCommandRequiresTarget`) to avoid the
need for checking this.
================
Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+ target->RegisterStats(StatisticKind::ExpressionSuccessful);
+ target->RegisterStats(StatisticKind::ExpressionFailure);
+ target->RegisterStats(StatisticKind::FrameVarSuccess);
+ target->RegisterStats(StatisticKind::FrameVarFailure);
----------------
Are you planning to have some kind of decentralized method of registering
statistics?
If not, then I don't see a need for explicitly registering the statistic kinds
here, since the single source of truth about the available kinds can be
StatisticKind enum, and Target can just get the list from there automatically
when need (when stats are enabled?)
This way we could even simplify the code by avoiding the "statistic is not
registered but someone still called IncrementStats" state altogether) and the
stats map could be a simple array with NumStatistics elements.
https://reviews.llvm.org/D45547
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
