[Lldb-commits] [PATCH] D97739: Add a progress class that can track and report long running operations that happen in LLDB.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This seems like the right way to go.  I made a couple trivial comment comments.

But also: the feature has the problem that when multiple debuggers are present, there's no way to tell what debugger actually triggered the work, and in that case all Debuggers will get the event.  That's not the Progress feature's fault, it's forced on us by the design of the global shared module cache.  But we shouldn't let the fact that that happens be first set of things for which you want to add progress drive the design.  Anyway, it seems to me like it would be simple to set this up to support other areas where there is a debugger, and doing that up front would keep people from accidentally adding progress but not providing the debugger because the API makes it seem like that isn't relevant.  I think the only things that you would need to add are:

1. Have Progress::ReportProgress take a Debugger * argument, and only use the static Debugger::ReportProgress when the debugger is NULL.  That would make it clear to users of the API that they should provide a Debugger if one is available.
2. Have another bit in the ProgressEventData that tells whether a debugger was supplied or not.  That way IDE's that support many debugger's running simultaneously would know to not associate anonymous progress events with any particular debugger, but to put it up in some kind of general progress.



================
Comment at: lldb/include/lldb/API/SBDebugger.h:50
+
+  /// Get progress data from a SBEvent that whose type is eBroadcastBitProgress.
+  ///
----------------
remove the "that"


================
Comment at: lldb/include/lldb/API/SBDebugger.h:59
+  /// \param [in] completed
+  ///   The amount of work completed. It \a completed is zero, then this event
+  ///   is a progress started event. If \a completed is equal to \a total, then
----------------
It -> If


================
Comment at: lldb/include/lldb/Core/Progress.h:66
+  /// @param [in] total The total units of work to be done if specified, if
+  /// set to UINT64_MAX there should be no progress displayed and just show a
+  /// spinning progress indicator.
----------------
Not grammatical.  Maybe "then an indeterminate progress indicator should be displayed".


================
Comment at: lldb/source/API/SBDebugger.cpp:855
+        error = m_opaque_sp->GetTargetList().CreateTarget(
+            *m_opaque_sp, filename, arch, eLoadDependentsYes, platform_sp,
+            target_sp);
----------------
These are just drive by reformattings, right?  Might be good not to include them in this change.


================
Comment at: lldb/source/Core/Debugger.cpp:1187
+
+void Debugger::ReportProgressPrivate(uint64_t progress_id,
+                                     const std::string &message,
----------------
Isn't ReportProgressPrivate the way that any part of the system that DOES have access to a Debugger should call to update Progress?  If so I don't think it's right to call this API "Private".  This should be documented as the preferred method, and the static ReportProgress should be documented as only to be used in situations where you can't find your way to a debugger.


================
Comment at: lldb/source/Core/Progress.cpp:49
+
+void Progress::ReportProgress() {
+  if (!m_complete) {
----------------
IMO it would be better for Progress::ReportProgress to take a Debugger *.  If the Debugger* is non-null, then we would call Debugger->ReportProgressPrivate or whatever is the appropriate name, and only if the Debugger is NULL would we call the static function.  It would be better if we have the right way to do things from the get-go, and since the Debugger is delivering the event, that's pretty clearly to call ReportProgress for the relevant Debugger.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1898
           Host::SystemLog(Host::eSystemLogError,
-                          "error: unable to find section %d for a symbol in %s, corrupt file?\n",
-                          n_sect, 
-                          filename.c_str());
+                          "error: unable to find section %d for a symbol in "
+                          "%s, corrupt file?\n",
----------------
Also an unrelated reformatting...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739