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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to