[Lldb-commits] [lldb] [lldb][progress] Add progress manager class (PR #81319)

[Greg Clayton via lldb-commits]

================
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
                              m_debugger_id);
   }
 }
+
+void ProgressManager::Initialize() {
+  lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+  InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+  lldbassert(InstanceImpl() &&
+             "A progress report manager has already been terminated.");
+  InstanceImpl().reset();
+}
+
+std::optional<ProgressManager> &ProgressManager::InstanceImpl() {
+  static std::optional<ProgressManager> g_progress_manager;
+  return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager &ProgressManager::Instance() { return *InstanceImpl(); }
----------------
clayborg wrote:

> We have other subsystems that work exactly like what Chelsea did here, such as the Diagnostics and the FileSystem. They rely on the requirement that you need to call `::Initialize ` before doing anything with it. It's true that this will crash if you forget to `Initialize`/`Terminate` but that's true for the `Debugger` too. This is an establishes pattern and has the advantage that it can be tested in unittest by initializing and terminating the subsystem between tests.

This is fine as long as you always check the optional then and we aren't doing that. So this can and will crash LLDB if extra threads call these APIs which is never good. So the API, in order to be safe, should be:
```
std::optional<ProgressManager &> ProgressManager::InstanceImpl() 
```
And then every user needs to check this optional. Otherwise, this:
```
ProgressManager &ProgressManager::Instance() { return *InstanceImpl(); }
```
Will crash if Terminate has been called right? the `std:optional::operator *`docs say "The behavior is undefined if *this does not contain a value." 

https://github.com/llvm/llvm-project/pull/81319