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