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