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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
