[Lldb-commits] [PATCH] D120972: [lldb] Show progress events in the command line driver

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Core/CoreProperties.td:137
+    DefaultTrue,
+    Desc<"Whether to show progress or not.">;
   def UseSourceCache: Property<"use-source-cache", "Boolean">,
----------------
Might be nice to clarify that this is for the CLI only? 

Also, if this _is_ for the CLI only, the setting should probably be put into the "interpreter" settings as "interpreter.show-progress".




================
Comment at: lldb/source/Core/Debugger.cpp:1756-1757
+  // can change between iterations so check it inside the loop.
+  if (!GetShowProgress())
+    return;
+
----------------
Move this to the top of the function so we don't do any work extracting anything from the event if it is disabled? Or is this code trying to limit the updates of a progress that reports many status updates for the same progress?


================
Comment at: lldb/source/Core/Debugger.cpp:1763
+  File &output = GetOutputFile();
+  if (!output.GetIsInteractive() || !output.GetIsTerminalWithColors())
+    return;
----------------
If not interactive should we just dump the start and end progress events on a separate line?


================
Comment at: lldb/source/Core/Debugger.cpp:1768
+    // Clear the current line.
+    output.Printf("\33[2K\r");
+    return;
----------------
Do we want some sort of format string here that the user could modify as a setting? The idea would be there might be extra settings that the user could set like:

(lldb) setting set interpreter.progress-clear-line-format "${ansi....}"

and it could default to the above string. Not required, just thinking out loud here as I am reading the patch


================
Comment at: lldb/source/Core/Debugger.cpp:1778-1779
+  if (data->GetTotal() != UINT64_MAX) {
+    output.Printf("[%llu/%llu] %s...", data->GetCompleted(), data->GetTotal(),
+                  message.c_str());
+  } else {
----------------
If we did a format string for each message we could have something like:

"{${progress.is_start}...}{${progress.is_update}...}{${progress.is_end}...}"

where the "progress.is_start" variable would be true for the first progress event, "progress.is_update" would be true for any updates, and "progress.is_end" would be true if the progress is completed. This would allow people to customize how progress events get handled and printed. If someone just wants a start and end progress, then they can fill in the "..." after the "progress.is_start" and "progress.is_end". If they don't want updates, they can leave out the "{${progress.is_update}...}" section. It also would allow ansi colors to be used since we already support these. And this would allow non interactive sessions to still show progress if they want to (right now if it isn't interactive, it doesn't get shown).


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

https://reviews.llvm.org/D120972