davide added a comment.
I prefer having it as a top level command rather than part of log. If you think
about it LLVM does the same distinction and it worked quite well in practice.
We plan to collect more metrics to the command so I'd very much like to have
this living as a separate entity.
================
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'])
----------------
labath wrote:
> Maybe test for failure as well (I guess trying to print an non-existing
> variable should do the trick)?
Testing for failure here is a little tricky because the expression parser
reports a failure only when it fails internally (and not when succeeds but
returns an error because clangAST can't find the variable). Eventually we could
make it more fine grained but for now I just would like to get the number of
failures resolving variable due to expression parsing logic failing (and not
because typos).
================
Comment at: lldb/source/Commands/CommandObjectStats.cpp:31-35
+ if (!target) {
+ result.AppendError("stats command needs a target");
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ }
----------------
labath wrote:
> I think we have some way (should involve `eCommandRequiresTarget`) to avoid
> the need for checking this.
I'll check this out, thanks!
================
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);
----------------
labath wrote:
> 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.
No. I originally thought to make it that way but I have to say that maybe it's
easier to have a centralized mechanism. I'm still unsure whether this should be
a vector or a map, I'll think about it more.
https://reviews.llvm.org/D45547
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
