Skip to content

minor ZSET code cleanup: Remove dead guard in deleteRangeCore and enhance buffer contracts - #4715

Open
rainsupreme wants to merge 1 commit into
valkey-io:unstablefrom
valkey-rainfall:oi/fbtree-score-size-annotations
Open

rainsupreme wants to merge 1 commit into
valkey-io:unstablefrom
valkey-rainfall:oi/fbtree-score-size-annotations

Conversation

@rainsupreme

Copy link
Copy Markdown
Contributor

deleteRangeCore handles only ranges whose two boundary leaves differ: fbtreeDeleteRangeByRank descends distinct children (li != ri) before calling it, and the value and score paths route same-leaf ranges to deleteRangeSameLeaf. The guard

if (bp->end_idx < 0 && bp->start_leaf == bp->end_leaf) return 0;

therefore never fires. It also sat directly under a comment explaining that no local empty-range short-circuit belongs at that point, so the code contradicted its own documentation. Replace it with an assert that states the precondition every caller already satisfies, and move the split_depth declaration to the block that uses it.

Every score-prefix parameter in fbtree.c now asserts that it must reference at least SCORE_SIZE readable bytes: read-only prefixes become const char name[static SCORE_SIZE], and the two exact-size output buffers in scoreRangeBounds are upgraded from [SCORE_SIZE] to [static SCORE_SIZE], so the contract is visible at every call site and compilers can diagnose undersized buffers.

fbtreeSeekToScore, fbtreeDeleteRangeByScore and fbtreeCountRangeByScore are public, but only their definitions are annotated. A C99 array parameter with a static size qualifier is invalid C++, and fbtree.h is included by test_fbtree.cpp, so the header prototypes stay const char *; C permits a definition to carry array qualifiers its declaration lacks.

No behavioral change.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 64ce0f54-9e1f-44fc-aa63-9c74f26a0289

📥 Commits

Reviewing files that changed from the base of the PR and between 5c24e5d and b3c260b.

📒 Files selected for processing (1)
  • src/fbtree.c

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The change adds static SCORE_SIZE array bounds to score-related parameters and replaces a same-leaf early return in deleteRangeCore with an assertion that boundary leaves differ.

Changes

Score Range Contracts

Layer / File(s) Summary
Score bounds and deletion invariant
src/fbtree.c
Score lookup, range, deletion, and counting functions now declare score buffers with static SCORE_SIZE bounds. deleteRangeCore now asserts distinct boundary leaves instead of returning for same-leaf ranges.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~4 minutes

Merge Risk: ⚪ Minimal · up to b3c26

The cleanup preserves the existing score and deletion contracts, so the change is mergeable with normal checks.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: removing the dead guard in deleteRangeCore and adding buffer contracts for ZSET score operations.
Description check ✅ Passed The description is directly related to the changeset and clearly explains the assertion replacement, buffer annotations, C++ compatibility, and lack of behavioral change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@rainsupreme
rainsupreme marked this pull request as ready for review September 16, 2026 18:16
@rainsupreme
rainsupreme marked this pull request as draft September 16, 2026 18:16

@valkey-review-bot valkey-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The public score-buffer annotations introduce warnings in the default server build because their header declarations still use pointer parameters.

Comment thread src/fbtree.c Outdated
…fix buffer contracts

deleteRangeCore handles only ranges whose two boundary leaves differ:
fbtreeDeleteRangeByRank descends distinct children (li != ri) before
calling it, and the value and score paths route same-leaf ranges to
deleteRangeSameLeaf. The guard

    if (bp->end_idx < 0 && bp->start_leaf == bp->end_leaf) return 0;

therefore never fires. It also sat directly under a comment explaining
that no local empty-range short-circuit belongs at that point, so the
code contradicted its own documentation. Replace it with an assert that
states the precondition every caller already satisfies, and restore the
split_depth declaration to the block that uses it.

Every score-prefix parameter of the static helpers in fbtree.c now
asserts that it must reference at least SCORE_SIZE readable bytes:
read-only prefixes become `const char name[static SCORE_SIZE]`, and the
two exact-size output buffers in scoreRangeBounds are upgraded from
`[SCORE_SIZE]` to `[static SCORE_SIZE]`, so the contract is visible at
every call site and compilers can diagnose undersized buffers.

The public fbtreeSeekToScore, fbtreeDeleteRangeByScore and
fbtreeCountRangeByScore keep `const char *`. Their prototypes in
fbtree.h must stay plain because a C99 array parameter with a `static`
size qualifier is invalid C++ and the header is included by
test_fbtree.cpp, and GCC 11+ rejects a definition whose array bound
differs from its prototype (-Werror=array-parameter).

No behavioral change.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
@rainsupreme
rainsupreme force-pushed the oi/fbtree-score-size-annotations branch from b3c260b to b0e0534 Compare September 16, 2026 18:29
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.82%. Comparing base (122ec7e) to head (b0e0534).
⚠️ Report is 2 commits behind head on unstable.

Files with missing lines Patch % Lines
src/fbtree.c 87.50% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #4715      +/-   ##
============================================
+ Coverage     80.63%   80.82%   +0.18%     
============================================
  Files           192      192              
  Lines        100803   100807       +4     
============================================
+ Hits          81287    81475     +188     
+ Misses        19516    19332     -184     
Files with missing lines Coverage Δ
src/fbtree.c 95.49% <87.50%> (ø)

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rainsupreme
rainsupreme marked this pull request as ready for review September 16, 2026 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant