fix(region,clenshaw): drop redundant >= 1 + break in unsigned loop (CodeQL #3512)#2921
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLoop condition in the ChangesClenshaw Evaluation Loop Optimization
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
b04871d to
1e72f18
Compare
…odeQL #3512) CodeQL flagged the inner `if (k == 1) break;` in clenshaw_eval as unreachable on the always-true condition `k >= 1` -- which is technically correct, since `std::size_t k` can never go negative. The original guard was added as defensive scaffolding against `k--` underflow, but the loop only ever runs k = N..1 (we exit before k == 0 would happen on the post-decrement), so the canonical idiom is `for (std::size_t k = N; k > 0; --k)`. Same semantics, identical generated code on Clang/GCC, no CodeQL nag. Originally raised as a coderabbit + CodeQL note on PR #2914 (merged in 709a715); this is the last unaddressed item from that review. No behaviour change; 404 assertions in the Region / SVDComponents Chebyshev tests still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1e72f18 to
edb9d72
Compare
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
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.
Summary
Follow-up to merged PR #2914 that addresses the one outstanding
review note (CodeQL alert #3512):
The Clenshaw recurrence iterates k = N down to 1 inclusive. The
original code used
for (std::size_t k = N; k >= 1; --k)with aninner
if (k == 1) break;to dodge unsigned underflow. CodeQLcorrectly noted that
k >= 1is always true for unsigned k --the break is the real loop exit, not the condition.
Canonical fix:
for (std::size_t k = N; k > 0; --k). Iterates k =N, N-1, ..., 1, exits cleanly before k = 0 would happen. Same
semantics, same generated code, CodeQL happy.
All other actionable review items on #2914 were already addressed
in commits during that PR (the coderabbit review thread shows ✅
markers on 12 of 13 comments); this one slipped through because it
was a separate CodeQL alert, not a coderabbit suggestion.
Test plan
./build_catch/CatchTestRunner "[Region][boundary_curve_cheb]"-- 404 assertions / 4 test cases pass🤖 Generated with Claude Code
Summary by CodeRabbit