[lldb][progress] Correctly check total for deterministic progress #79912
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Chelsea Cassanova (chelcassanova) ChangesThe The member variable Full diff: https://github.com/llvm/llvm-project/pull/79912.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Core/DebuggerEvents.h b/lldb/include/lldb/Core/DebuggerEvents.h
index 4a27766e94e3a..2c3fcd7069d5e 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -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; }
uint64_t GetCompleted() const { return m_completed; }
uint64_t GetTotal() const { return m_total; }
std::string GetMessage() const {
diff --git a/lldb/source/Core/DebuggerEvents.cpp b/lldb/source/Core/DebuggerEvents.cpp
index dd77fff349a64..c83bc20fba97a 100644
--- a/lldb/source/Core/DebuggerEvents.cpp
+++ b/lldb/source/Core/DebuggerEvents.cpp
@@ -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)
s->Printf(", progress = %" PRIu64 " of %" PRIu64, m_completed, m_total);
}
|
The `total` parameter for the constructor for Progress was changed to a std::optional in llvm#77547. When initializing the `m_total` member variable for progress, it is set to 1 if the `total` parameter is std::nullopt. Other areas of the code were still checking if `m_total` was a UINT64_MAX to determine if the progress was deterministic or not, so these have been changed to check for the integer 1. The member variable `m_total` could be changed to a std::optional as well, but this means that the `ProgressEventData::GetTotal()` (which is used for the public API) would either need to return a std::optional value or it would return some specific integer to represent non-deterministic progress if `m_total` is std::nullopt.
chelcassanova
force-pushed
the
progress-change-total-check
branch
from
1f16436 to
af9a558
Compare
clayborg
reviewed
Jan 30, 2024
| @@ -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; } | |||
There was a problem hiding this comment.
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; }
There was a problem hiding this comment.
I realize I had suggested using the magic number 1 in previous reviews...
There was a problem hiding this comment.
I added your suggestions to the PR, no longer using a magic number or the number 1 to represent non-deterministic progress
lldb/source/Core/DebuggerEvents.cpp
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
if (m_total != Progress::kNonDeterministicTotal)
lldb/source/Core/Progress.cpp
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
m_total(kNonDeterministicTotal) {
JDevlieghere
approved these changes
Jan 30, 2024
Uses a static class member set to UINT64_MAX instead of using a magic number of 1 to represent non-deterministic progress.
chelcassanova
added a commit
to chelcassanova/llvm-project
that referenced
this pull request
Jan 30, 2024
Use the changes implemented in llvm#79912 to use a static variable from `Progress` to ensure that the report was created with a non-deterministic total.
chelcassanova
added a commit
to chelcassanova/llvm-project
that referenced
this pull request
Jan 30, 2024
Use the changes implemented in llvm#79912 to use a static variable from `Progress` to ensure that the report was created with a non-deterministic total.
chelcassanova
added a commit
to chelcassanova/llvm-project
that referenced
this pull request
Jan 31, 2024
Use the changes implemented in llvm#79912 to use a static variable from `Progress` to ensure that the report was created with a non-deterministic total.
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Feb 1, 2024
…vm#79912) The `total` parameter for the constructor for Progress was changed to a std::optional in llvm#77547. It was originally set to 1 to indicate non-determinisitic progress, but this commit changes this. First, `UINT64_MAX` will again be used for non-deterministic progress, and `Progress` now has a static variable set to this value so that we can use this instead of a magic number. The member variable `m_total` could be changed to a std::optional as well, but this means that the `ProgressEventData::GetTotal()` (which is used for the public API) would either need to return a std::optional value or it would return some specific integer to represent non-deterministic progress if `m_total` is std::nullopt. (cherry picked from commit 733b86d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
totalparameter for the constructor for Progress was changed to a std::optional in #77547. When initializing them_totalmember variable for progress, it is set to 1 if thetotalparameter is std::nullopt. Other areas of the code were still checking ifm_totalwas a UINT64_MAX to determine if the progress was deterministic or not, so these have been changed to check for the integer 1.The member variable
m_totalcould be changed to a std::optional as well, but this means that theProgressEventData::GetTotal()(which is used for the public API) wouldeither need to return a std::optional value or it would return some specific integer to represent non-deterministic progress if
m_totalis std::nullopt.