[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Pavel Labath via Phabricator via lldb-commits]

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