Skip to content

fix: make the wrapped successor explicit in CRangesSet under -fsanitize=integer #7590

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
PastaPastaPasta merged 1 commit into from
Aug 13, 2026
Merged

fix: make the wrapped successor explicit in CRangesSet under -fsanitize=integer #7590

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 14 additions & 4 deletions src/util/ranges_set.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,16 @@

#include <limits>

namespace {
//! Successor of a value in the half-open range encoding: the range containing
//! UINT64_MAX stores a wrapped end of 0. Spelled as a branch so the wrap is
//! explicit intent rather than arithmetic overflow (-fsanitize=integer).
constexpr uint64_t WrappedSuccessor(uint64_t value) noexcept
{
return value == std::numeric_limits<uint64_t>::max() ? 0 : value + 1;
}
} // namespace

CRangesSet::Range::Range() : CRangesSet::Range::Range(0, 0) {}

CRangesSet::Range::Range(uint64_t begin_in, uint64_t end_in) :
Expand All @@ -24,7 +34,7 @@ bool CRangesSet::Add(uint64_t value)
// all 3 of them should be merged in one range [x, y)
// - if there's exist a range [x, value) - we need to replace it to new range [x, value + 1)
// - if there's exist a range [value + 1, y) - we need to replace it to new range [value, y)
Comment on lines 34 to 36

@thepastaclaw thepastaclaw Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 Nitpick: Describe the wrapped successor in Add() comments

The implementation now deliberately uses WrappedSuccessor(value), but the adjacent algorithm comments still describe every endpoint as value + 1. For UINT64_MAX, the stored endpoint is actually 0, so these comments obscure the boundary behavior this PR makes explicit. Use the helper's name in the range descriptions and document its wrapped endpoint.

Suggested change
// all 3 of them should be merged in one range [x, y)
// - if there's exist a range [x, value) - we need to replace it to new range [x, value + 1)
// - if there's exist a range [value + 1, y) - we need to replace it to new range [value, y)
// If element is not in CRangesSet, add the encoded range
// [value, WrappedSuccessor(value)); UINT64_MAX uses a wrapped end of 0.
// This operation can cause two merges (three cases):
// - if ranges [x, value) and [WrappedSuccessor(value), y) exist,
// merge all three ranges into [x, y)
// - if [x, value) exists, extend it through value
// - if [WrappedSuccessor(value), y) exists, prepend value to it

source: ['coderabbit']

Range new_range{value, value + 1};
Range new_range{value, WrappedSuccessor(value)};

@coderabbitai coderabbitai Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the adjacent Add() range comment.

At Line 37, Range new_range uses WrappedSuccessor(value), but the comment still describes [value, value + 1) unconditionally. For UINT64_MAX, the stored representation is {UINT64_MAX, 0}. Reference WrappedSuccessor(value) or document this wrap explicitly.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/util/ranges_set.cpp` at line 37, Update the comment adjacent to Range
new_range in Add() to describe the endpoint using WrappedSuccessor(value),
including the UINT64_MAX case where the representation wraps to 0 instead of
always claiming [value, value + 1).

auto it = ranges.lower_bound({value, value});
if (it != ranges.begin()) {
auto prev = it;
Expand All @@ -37,7 +47,7 @@ bool CRangesSet::Add(uint64_t value)
}
const auto next = it;
if (next != ranges.end()) {
if (next->begin == value + 1) {
if (next->begin == WrappedSuccessor(value)) {
new_range.end = next->end;
ranges.erase(next);
}
Expand Down Expand Up @@ -67,8 +77,8 @@ bool CRangesSet::Remove(uint64_t value)
const auto ret = ranges.insert({current_range.begin, value});
assert(ret.second);
}
if (value + 1 != current_range.end) {
const auto ret = ranges.insert({value + 1, current_range.end});
if (WrappedSuccessor(value) != current_range.end) {
const auto ret = ranges.insert({WrappedSuccessor(value), current_range.end});
assert(ret.second);
}
return true;
Expand Down
Loading