Skip to content

[lldb][progress] Correctly check total for deterministic progress #79912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 30, 2024
Merged

[lldb][progress] Correctly check total for deterministic progress #79912

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af9a558
[lldb][progress] Correctly check total for deterministic progress
chelcassanova Jan 29, 2024
466836d
Use UINT64_MAX, use static class var instead of magic number
chelcassanova Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lldb/include/lldb/Core/DebuggerEvents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class ProgressEventData : public EventData {
GetAsStructuredData(const Event *event_ptr);

uint64_t GetID() const { return m_id; }
bool IsFinite() const { return m_total != UINT64_MAX; }
bool IsFinite() const { return m_total != 1; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to actually hard code a constant value inside the progress class and use it. We are using magic numbers.

class Progress {
constexpr uint64_t kNonDeterministicTotal = UINT64_MAX;

I was also thinking that UINT64_MAX might be a better value to use than 1 because people can create valid deterministic Progress with 1 work item. So this code would be:

bool IsFinite() const { return m_total != kNonDeterministicTotal; }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize I had suggested using the magic number 1 in previous reviews...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added your suggestions to the PR, no longer using a magic number or the number 1 to represent non-deterministic progress

uint64_t GetCompleted() const { return m_completed; }
uint64_t GetTotal() const { return m_total; }
std::string GetMessage() const {
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Core/DebuggerEvents.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ void ProgressEventData::Dump(Stream *s) const {
s->PutCString(", type = update");
// If m_total is UINT64_MAX, there is no progress to report, just "start"
// and "end". If it isn't we will show the completed and total amounts.
if (m_total != UINT64_MAX)
if (m_total != 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if (m_total != Progress::kNonDeterministicTotal)

s->Printf(", progress = %" PRIu64 " of %" PRIu64, m_completed, m_total);
}

Expand Down
1 change: 0 additions & 1 deletion lldb/source/Core/Progress.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ Progress::Progress(std::string title, std::string details,
lldb_private::Debugger *debugger)
: m_title(title), m_details(details), m_id(++g_id), m_completed(0),
m_total(1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

m_total(kNonDeterministicTotal) {

assert(total == std::nullopt || total > 0);
if (total)
m_total = *total;

Expand Down