[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 a comment.
This revision now requires changes to proceed.

The only last nit is we are passing the report progress callback around all over the place. I think it would be nicer to just make a static function that we can call and remove the passing of the instance variable for the report callback all over to the event classes.



================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:27
+                             const ProgressEvent *prev_event)
+    : m_progress_id(progress_id), m_message(message.str()) {
+  if (completed == total) {
----------------
So we are copying the string for every event? I thought we only needed this for the start event?


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:28-57
+  if (completed == total) {
+    m_percentage = 100;
+  } else if (total != UINT64_MAX) {
+    m_percentage = std::min(
+        (uint32_t)((long double)completed / (long double)total * 100.0),
+        (uint32_t)99);
+  }
----------------
I would be nice to simplify this down a bit as we have both "completed" being used along with "prev_event" to try and figure out what the event is. We probably don't need "long double" for calculating the percentage either. It would be clearer to maybe do:

```
const bool calculate_percentage = total != UINT64_MAX;
if (completed == 0) {
  // Start event
  m_event_type = progressStart;
  // Wait a bit before reporting the start event in case in completes really quickly.
  m_minimum_allowed_report_time = m_creation_time + kStartProgressEventReportDelay;
  if (calculate_percentage)
    m_precentage = 0;
} else if (completed == total) {
  // End event
  m_event_type = progressEnd;
  // We should report the end event right away.
  m_minimum_allowed_report_time = std::chrono::seconds::zero();
  if (calculate_percentage)
    m_precentage = 100;
} else {
  // Update event
  assert(prev_event);
  m_percentage = std::min(
        (uint32_t)((double)completed / (double)total * 100.0),
        (uint32_t)99);
  if (prev_event->Reported()) {
    // Add a small delay between reports
    m_minimum_allowed_report_time =
        prev_event->m_minimum_allowed_report_time +
        kUpdateProgressEventReportDelay; 
  } else {
    // We should use the previous timestamp, as it's still pending
    m_minimum_allowed_report_time =
        prev_event->m_minimum_allowed_report_time; 
  }
}
```


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:147
+  if (m_last_update_event)
+    m_last_update_event->Report(m_report_callback);
+  return true;
----------------
Should we return the result of "m_last_update_event->Report(m_report_callback);" here?


================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:157
+                                  uint64_t completed, uint64_t total) {
+  if (Optional<ProgressEvent> event = ProgressEvent::Create(
+          progress_id, message, completed, total, &GetMostRecentEvent())) {
----------------
Should we add a safeguard and check m_finished first?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101128