[Lldb-commits] [PATCH] D111686: Modify "statistics dump" to dump JSON.

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/Process.h:241
 
-  void BumpStopID() {
-    m_stop_id++;
+  uint32_t BumpStopID() {
+    const uint32_t prev_stop_id = m_stop_id++;
----------------
add a comment saying that this returns the stop id with the value before the bump, otherwise someone might think that this returns the modified value


================
Comment at: lldb/include/lldb/Target/Statistics.h:23
+struct SuccessFailStats {
+  void Notify(bool success);
+
----------------
JDevlieghere wrote:
> I'd use an enum class with `Success` and `Failure` here instead of a bool that requires you to know what the argument is named to know if true means success of failure. 
Or maybe just NotifySuccess(bool) to keep callers simple


================
Comment at: lldb/include/lldb/Target/Target.h:1466-1468
+  /// \param [in] modules
+  ///     If true, include an array of metrics for each module loaded in the
+  ///     target.
----------------
this param is not present


================
Comment at: lldb/include/lldb/Utility/ElapsedTime.h:22
+///
+/// Objects that need to measure elapsed times should have a variable with of
+/// type "ElapsedTime::Duration m_time_xxx;" which can then be used in the
----------------



================
Comment at: lldb/include/lldb/Utility/ElapsedTime.h:29
+///
+/// This class will update the m_time_xxx variable with the elapsed time when
+/// the object goes out of scope. The "m_time_xxx" variable will be incremented
----------------



================
Comment at: lldb/include/lldb/Utility/ElapsedTime.h:41
+  Timepoint m_start_time;
+  /// The elapsed time in seconds to update when this object goes out of scope.
+  ElapsedTime::Duration &m_elapsed_time;
----------------



================
Comment at: lldb/include/lldb/lldb-forward.h:121
 class MemoryRegionInfos;
+struct StatsDumpOptions;
 class Module;
----------------
move this to line ~277


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:761
 
+    def getShlibBuildArtifact(self, DYLIB_NAME):
+        """Return absolute path to a shared library artifact given the library
----------------
is this actually used somewhere?


================
Comment at: lldb/source/API/SBTarget.cpp:216-220
+  std::string json_text;
+  llvm::raw_string_ostream stream(json_text);
+  llvm::json::Value json = target_sp->ReportStatistics();
+  stream << json;
+  data.m_impl_up->SetObjectSP(StructuredData::ParseJSON(stream.str()));
----------------
a simpler version


================
Comment at: lldb/source/Commands/CommandObjectExpression.cpp:662-668
     // Increment statistics to record this expression evaluation success.
-    target.IncrementStats(StatisticKind::ExpressionSuccessful);
+    target.GetStatistics().NotifyExprEval(true);
     return true;
   }
 
   // Increment statistics to record this expression evaluation failure.
+  target.GetStatistics().NotifyExprEval(false);
----------------
actually this is not sufficient, we need to log as well in the SBAPI side, which is what lldb-vscode uses to evaluate expressions. So either move it to deeper point in the call chain or report this event from other locations


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:711
     bool res = result.Succeeded();
-    Target &target = GetSelectedOrDummyTarget();
-    if (res)
-      target.IncrementStats(StatisticKind::FrameVarSuccess);
-    else
-      target.IncrementStats(StatisticKind::FrameVarFailure);
+    GetSelectedOrDummyTarget().GetStatistics().NotifyFrameVar(res);
     return res;
----------------
same here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111686/new/

https://reviews.llvm.org/D111686