[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 inline comments.


================
Comment at: lldb/include/lldb/API/SBDebugger.h:46
 
+  static const char *GetBroadcasterClass();
+
----------------
clayborg wrote:
> @jingham: do we need this GetBroadcasterClass()? Or would this only be used to listen to all debugger events as new debuggers are created? Also see my question later that asks if we need the broadcast manager in the Broadcaster we are using in the Debugger.h/Debugger.cpp.
It is needed by the mechanism for waiting for the events from all broadcasters of a given type.  Not sure why you wouldn't want to do this for Debugger events.

GetBroadcasterClass is used in the Debugger DefaultEventHandler to separate the treatment for the various event sources the Debugger waits for.  One of the missing pieces of this patch is to get the Driver's default event handling to do something with progress updates.  I presume that would go through the same DefaultEventHandler, and you'd use the comparison of GetStaticBroadcasterClass and GetBroadcasterClass in the same way that code does for all the other event classes.


================
Comment at: lldb/source/Core/Debugger.cpp:672
       Properties(std::make_shared<OptionValueProperties>()),
+      Broadcaster(nullptr, GetStaticBroadcasterClass().AsCString()),
       m_input_file_sp(std::make_shared<NativeFile>(stdin, false)),
----------------
clayborg wrote:
> @jingham: do we need to the m_broadcaster_manager_sp in this broadcaster? Debugger inherits from Broadcaster, but we could change this to "has a" instead of "is a" if we need the broadcast manager. 
Given that at least in the first pass all the events are not specific to a debugger, in a Multi-Debugger UI I could very well imagine that instead of having any individual debugger's Listener listen for these events, you would make some thread listen for them all, and then put up some System Wide progress UI.  That would make it much less confusing than seeming to associate events with debuggers you don't know they pertain to.  

And even when we start reporting Progress that was associated with a Debugger, we will still have all these unassociated events from the module cache.  So you could still imagine having a listener for all debuggers, that only reports progress that has no debugger associated with it, and then have individual Debugger's report progress pertaining directly to them.

So I do think we need to have a BroadcasterManager that allows you to sign up for all Debugger events from any debugger that will be created.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739