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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Thanks for the review Greg!



================
Comment at: lldb/source/Core/CoreProperties.td:137
+    DefaultTrue,
+    Desc<"Whether to show progress or not.">;
   def UseSourceCache: Property<"use-source-cache", "Boolean">,
----------------
clayborg wrote:
> 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".
> 
> 
I've updated the description to make it clear that this hinges of the debugger output being an interactive color enabled terminal.


================
Comment at: lldb/source/Core/Debugger.cpp:1756-1757
+  // can change between iterations so check it inside the loop.
+  if (!GetShowProgress())
+    return;
+
----------------
clayborg wrote:
> 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?
Kind of. `m_current_event_id` ensures that we only deal with one event at the same time. If we moved this check before the `m_current_event_id` bookkeeping, someone could disable progress before the current progress event completes resulting in `m_current_event_id` never getting cleared.

For example:

```
event 1 begin -> m_current_event_id = 1
progress disabled 
event 1 end -> ignored
progress enabled
event 2 begin -> ignored because m_current_event_id == 1
and now every subsequent event is ignored because m_current_event_id will never get updated 
```


================
Comment at: lldb/source/Core/Debugger.cpp:1763
+  File &output = GetOutputFile();
+  if (!output.GetIsInteractive() || !output.GetIsTerminalWithColors())
+    return;
----------------
clayborg wrote:
> If not interactive should we just dump the start and end progress events on a separate line?
I (personally) think that will quickly generate too much output.


================
Comment at: lldb/source/Core/Debugger.cpp:1768
+    // Clear the current line.
+    output.Printf("\33[2K\r");
+    return;
----------------
clayborg wrote:
> 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
Part of that is covered in https://reviews.llvm.org/D121062. 

I didn't make the vt100 escape codes configurable. They're the same as what editline uses (which aren't configurable either) and to me are just implementation details.


================
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 {
----------------
clayborg wrote:
> 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).
I intentionally kept things simple for now, but this is definitely something we could make more sophisticated/configurable in the future.


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

https://reviews.llvm.org/D120972