[Lldb-commits] [PATCH] D97739: Add a progress class that can track and report long running operations that happen in LLDB.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Very cool, this is something I have wanted for a long time. Another really good use case here would be `dSYMForUUID` which can take such a long time that people think the debugger hangs. I think this will be really powerful on the command line too if we can hook this up with a little progress bar that appears and disappears during long running tasks. I left a few comments inline.



================
Comment at: lldb/include/lldb/Core/Progress.h:1
+//===-- Progress.h ----------------------------------------------*- C++ -*-===//
+//
----------------
I think this belongs more in Utility than Core. 


================
Comment at: lldb/include/lldb/Core/Progress.h:19
+public:
+  Progress(uint64_t total, const char *format, ...)
+      __attribute__((format(printf, 3, 4)));
----------------
I'm not a fan of how we'rere-implementing printf across different utility classes. Can we have this take  `std::string` (that we move into the member) and have the caller use `formatv` or something to construct the string? 


================
Comment at: lldb/include/lldb/Core/Progress.h:24
+
+  static void SetProgressCallback(lldb::ProgressCallback callback, void *baton);
+
----------------
Rather than making this a static property of the `Progress` class, maybe this should be a member of the debugger? And then you could have a factory in the debugger `CreateProgress` that hands out a new `Progress` instance. The reason I'd like to have this tied to the debugger is that I imagine using this to show a little progress bar on the command line. When we do that we can have the debugger wrap its own callback around the one provided by the user instead of having to hijack it. It would also align with where the function is exposed in the SB API. 


================
Comment at: lldb/include/lldb/Core/Progress.h:30
+  static lldb::ProgressCallback g_callback;
+  static void *g_callback_baton;
+  ConstString m_message;
----------------
What do you imagine this baton to be used for?


================
Comment at: lldb/include/lldb/Core/Progress.h:32
+  ConstString m_message;
+  std::atomic<uint32_t> m_completed;
+  const uint64_t m_total;
----------------
Why not `uint64_t`? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739