[Lldb-commits] [PATCH] D101128: [lldb-vscode] only report long running progress events

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:143-148
+  // The event finished before we were able to report it.
+  if (!m_start_event.Reported() && Finished())
+    return true;
+
+  if (!m_start_event.Report(m_report_callback))
+    return false;
----------------



================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:171
+
+bool ProgressEventManager::Finished() const { return m_finished; }
+
----------------
put in header file


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:176
+    : m_report_callback(report_callback) {
+  m_thread_should_exit = false;
+  m_thread = std::thread([&] {
----------------
init before { with "m_thread_should_exit(false)"


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:197-201
+    else if (event_manager->ReportIfNeeded())
+      m_unreported_start_events
+          .pop(); // we remove it from the queue as it started reporting
+                  // already, the Push method will be able to continue its
+                  // reports.
----------------



================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:214-215
+  if (it == m_event_managers.end()) {
+    if (Optional<ProgressEvent> event =
+            CreateStartProgressEvent(progress_id, message, completed, total)) {
+      ProgressEventManagerSP event_manager =
----------------
Why is this returning an optional event? We know we need a start event here and it must be made. The Optional seems like it isn't needed?


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:31-40
+  ProgressEvent(uint64_t progress_id, llvm::StringRef message,
+                llvm::Optional<uint32_t> percentage);
 
-  ProgressEvent(uint64_t progress_id, const char *message, uint64_t completed,
-                uint64_t total);
+  /// Constructor for an update or end event
+  ProgressEvent(uint64_t progress_id, const ProgressEvent &prev_event);
+
+  /// Constructor for an update or end event, which happens when percentage is
----------------
Seems like all these constructors are a bit much. We should probably have one that just takes all the needed parameters including the ProgressEventType. It is unclear from each constructor what each one does unless you look very carefully at the header documentation. It will be very clear if we just have:

```
ProgressEvent(uint64_t progress_id, StringRef message, ProgressEventType event_type, llvm::Optional<uint32_t> percentage = llvm::None);
```


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:32
+  ProgressEvent(uint64_t progress_id, llvm::StringRef message,
+                llvm::Optional<uint32_t> percentage);
 
----------------
Why is there an optional percentage for a start event?


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:35
+  /// Constructor for an update or end event
+  ProgressEvent(uint64_t progress_id, const ProgressEvent &prev_event);
+
----------------
Why do we need this constructor? At the very least we don't need the progress_id since the prev_event will have one


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:39-40
+  /// 100.
+  ProgressEvent(uint64_t progress_id, uint32_t percentage,
+                const ProgressEvent &prev_event);
 
----------------
Don't need progress_id if we have a prev_event. We probably don't need the previous constructor if we have this one. Might be better as:

```
ProgressEvent(const ProgressEvent &prev_event, uint32_t percentage);
```
If the percentage is 100, then this is an end event? Otherwise it is an update event?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101128