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 
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.

lldb-commits mailing list

Reply via email to