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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
