clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
The only last nit is we are passing the report progress callback around all
over the place. I think it would be nicer to just make a static function that
we can call and remove the passing of the instance variable for the report
callback all over to the event classes.
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:27
+ const ProgressEvent *prev_event)
+ : m_progress_id(progress_id), m_message(message.str()) {
+ if (completed == total) {
----------------
So we are copying the string for every event? I thought we only needed this for
the start event?
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:28-57
+ if (completed == total) {
+ m_percentage = 100;
+ } else if (total != UINT64_MAX) {
+ m_percentage = std::min(
+ (uint32_t)((long double)completed / (long double)total * 100.0),
+ (uint32_t)99);
+ }
----------------
I would be nice to simplify this down a bit as we have both "completed" being
used along with "prev_event" to try and figure out what the event is. We
probably don't need "long double" for calculating the percentage either. It
would be clearer to maybe do:
```
const bool calculate_percentage = total != UINT64_MAX;
if (completed == 0) {
// Start event
m_event_type = progressStart;
// Wait a bit before reporting the start event in case in completes really
quickly.
m_minimum_allowed_report_time = m_creation_time +
kStartProgressEventReportDelay;
if (calculate_percentage)
m_precentage = 0;
} else if (completed == total) {
// End event
m_event_type = progressEnd;
// We should report the end event right away.
m_minimum_allowed_report_time = std::chrono::seconds::zero();
if (calculate_percentage)
m_precentage = 100;
} else {
// Update event
assert(prev_event);
m_percentage = std::min(
(uint32_t)((double)completed / (double)total * 100.0),
(uint32_t)99);
if (prev_event->Reported()) {
// Add a small delay between reports
m_minimum_allowed_report_time =
prev_event->m_minimum_allowed_report_time +
kUpdateProgressEventReportDelay;
} else {
// We should use the previous timestamp, as it's still pending
m_minimum_allowed_report_time =
prev_event->m_minimum_allowed_report_time;
}
}
```
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:147
+ if (m_last_update_event)
+ m_last_update_event->Report(m_report_callback);
+ return true;
----------------
Should we return the result of
"m_last_update_event->Report(m_report_callback);" here?
================
Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:157
+ uint64_t completed, uint64_t total) {
+ if (Optional<ProgressEvent> event = ProgressEvent::Create(
+ progress_id, message, completed, total, &GetMostRecentEvent())) {
----------------
Should we add a safeguard and check m_finished first?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101128/new/
https://reviews.llvm.org/D101128
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
