Repository navigation
ruff_cpp_spo: tag for-header slots (for_init/for_cond/for_inc) from the real ; tokens - #116
Merged
Merged
Conversation
…kens The ordered ore stream could not tell which of a ForStmt's header children was the initializer, the condition, or the increment: libclang exposes the ForStmt's children positionally and omits absent slots, so `for (;; ++i)` and `for (i = 0;;)` both present a single header child. Tokenize the ForStmt's own extent, find the two depth-1 `;` separators, and tag every event whose offset falls inside a slot with `for_init` / `for_cond` / `for_inc` when it carries no other control tag. Headers whose depth-1 `;` count is not exactly two (macro-generated headers) are left untagged rather than guessed. Re-harvest on ladybug `src/` is row-identical (646,411 events); slot tag counts: for_init 3,053 / for_cond 6,748 / for_inc 2,880; 9 headers declined. Eight unit tests; six of them fail with the slot arm disabled. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JRoXmYPrE8KubXVrDB71L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughThe change classifies C++ ChangesFor-loop header classification
Priority: ⬇️ Low — Defer the for-loop tagging change because it is a contained parser enhancement with no stated external urgency or broader product impact. Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change adds positional init, condition, and increment tags for valid C++ for-loop headers while preserving existing annotations and leaving unsupported headers untagged. No current merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
A rabbit sorts the loop’s three tracks Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a96c6b14-052f-4713-ad76-537a2be23165) |
AdaWorldAPI
marked this pull request as ready for review
September 8, 2026 13:41
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7d37b8f3-a680-4c2d-abe5-bcd77e7c1495) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 572a7cffc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…skip braced `;`, tokenize the header only Three findings from the Codex review of #116, each reproduced as a failing test before the fix: 1. A loop emitted whole by a function-like macro (`#define LOOP(I,C,N) for (I; C; N)`) tokenizes as the macro DEFINITION's tokens, so the two `;` matched while the head-child anchors pointed at the expansion site and every event fell through to `for_inc`. Every separator must now lie strictly between the `for` keyword's expansion-site offset and the body's; a header spelled elsewhere yields `None` (untagged), never a mis-tag. 2. A `;` inside a braced expression in the header (a lambda body in the init clause) counted as a separator, so a valid header returned `None`. Brace depth is tracked; only `;` at paren depth 1 and brace depth 0 count. 3. The whole `ForStmt` extent was tokenized (including the body, re-lexed once per enclosing loop). Only the header range — `for` keyword to the body's first token — is tokenized now. Gate on the ladybug corpus (`ore-full-v3` vs `ore-full-v2`): 646,411 rows, byte-identical in every column except `control`; no tag added or moved; 158 tags removed in 34 methods, all in `third_party/roaring_bitmap` (132 `for_inc`, 15 `for_cond`, 11 `for_init`) — the finding-1 corruption, now declined. Slot counts: for_init 3,053→3,042, for_cond 6,748→6,733, for_inc 2,880→2,748. Two new tests (`for_macro_emitted_loop_gets_no_slot_tags`, `for_lambda_in_init_still_tags_all_three_slots`); each fails with its guard removed and passes with it. 68 lib + 6 example tests, clippy -D warnings, fmt clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JRoXmYPrE8KubXVrDB71L
… untag it The macro-rejection fix in 15624df over-rejected. It bounded the header by the body cursor's raw location, and when the body is itself a macro invocation that location is a macro-expansion location: libclang tokenizes such a range to a single token (measured, ntok=1), so the separator count is never two and every slot tag on an ordinary header is silently dropped. Measured on the ladybug corpus (1,081 TUs, 646,411 rows): 158 tags in 34 methods went away, but only 31 of those methods are genuinely macro-emitted. The other three are ordinary headers whose body is a macro call — `for (...) LOG(ERROR) << x;` in two re2 methods and `for (...) TDEFL_PUT_BITS(...);` in miniz. Both range endpoints are now materialised as file locations in the `for` keyword's own file via `File::get_offset_location`, which keeps the header-only range (no ancestor re-lexes a descendant body) and makes it well-formed whatever the body is. That also subsumes the per-separator offset check, which is deleted rather than left as an unfalsifiable guard: for a wholly macro-emitted loop the materialised range spans the CALL SITE, whose tokens carry no `;` at header depth, so the count is not two and the header is rejected without ever reading a definition offset. Gate, v2 (pre-15624df) against this commit, same corpus, same extractor otherwise: 646,411 rows both sides; 100 tags removed across 31 methods, 0 added, 0 moved; `for_init` and `for_cond` counts unchanged at 3,053 and 6,748; `for_inc` 2,880 -> 2,754. The 126 removed `for_inc` tags are exactly the macro-emitted separators that used to fall through to that slot, which is the failure the review named. 30 of the 31 methods are macro loops in `third_party/roaring_bitmap/roaring.c`, the 31st is mbedTLS' `SHIFT_AND_PLACE`; no non-macro loop declines. Tests: `for_with_a_macro_body_still_tags_all_three_slots` (disable-verified against the raw-location endpoints) and `for_emitted_whole_with_its_body_gets_no_slot_tags`, which pins the shape `LOOP` does not cover — header and body from one object-like macro. The range-validity precondition is labelled as such, since no fixture reaches it. Also reword a doc comment so the `typos` hook stops failing on `mis-tagged`. 70 lib + 6 example tests pass; clippy -D warnings and fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JRoXmYPrE8KubXVrDB71L
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4737c418-d1cd-4b5e-bc0e-b538ed02c767) |
AdaWorldAPI
pushed a commit
that referenced
this pull request
Oct 14, 2026
…skip braced `;`, tokenize the header only Three findings from the Codex review of #116, each reproduced as a failing test before the fix: 1. A loop emitted whole by a function-like macro (`#define LOOP(I,C,N) for (I; C; N)`) tokenizes as the macro DEFINITION's tokens, so the two `;` matched while the head-child anchors pointed at the expansion site and every event fell through to `for_inc`. Every separator must now lie strictly between the `for` keyword's expansion-site offset and the body's; a header spelled elsewhere yields `None` (untagged), never a mis-tag. 2. A `;` inside a braced expression in the header (a lambda body in the init clause) counted as a separator, so a valid header returned `None`. Brace depth is tracked; only `;` at paren depth 1 and brace depth 0 count. 3. The whole `ForStmt` extent was tokenized (including the body, re-lexed once per enclosing loop). Only the header range — `for` keyword to the body's first token — is tokenized now. Gate on the ladybug corpus (`ore-full-v3` vs `ore-full-v2`): 646,411 rows, byte-identical in every column except `control`; no tag added or moved; 158 tags removed in 34 methods, all in `third_party/roaring_bitmap` (132 `for_inc`, 15 `for_cond`, 11 `for_init`) — the finding-1 corruption, now declined. Slot counts: for_init 3,053→3,042, for_cond 6,748→6,733, for_inc 2,880→2,748. Two new tests (`for_macro_emitted_loop_gets_no_slot_tags`, `for_lambda_in_init_still_tags_all_three_slots`); each fails with its guard removed and passes with it. 68 lib + 6 example tests, clippy -D warnings, fmt clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JRoXmYPrE8KubXVrDB71L
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
The ordered ore stream (
ruff_cpp_spo::events) could not tell which of aForStmt's header children was the initializer, the condition, or the increment. libclang exposes the header children positionally and omits absent slots, sofor (;; ++i)andfor (i = 0;;)both present a single header child, and a downstream consumer that wants to rotate the increment after the body (the control-normalized view used by the behavioral-motif lab) has nothing to key on.This change tokenizes the
forheader, finds the two depth-1;separators, and tags every event whose offset falls inside a slot withfor_init/for_cond/for_inc— only when the event carries no other control tag, soif/switch/ loop tags inside the header are never overwritten. A header whose depth-1;count is not exactly two is left untagged rather than guessed.Nothing else in the stream changes: the raw event sequence, scope kinds, anchors, and every existing control tag are untouched.
Review round
Three review findings landed on the first commit, and addressing the P1 exposed a fourth defect that the same review's P2 had warned about from the other side. All four are fixed; the threads carry the detail.
for_inc. Measured on the corpus: 126 badfor_inctags and zero badfor_init/for_cond, exactly the skew the finding predicted.;in the header. A lambda body ininitcounted its own;as a separator. Fixed with a brace-depth counter beside the paren counter.ForStmtextent was tokenized, re-lexing descendant bodies once per ancestor. The range is nowforkeyword to body start.for (...) LOG(ERROR) << x;in re2,for (...) TDEFL_PUT_BITS(...);in miniz). Both endpoints are now materialised as file locations in theforkeyword's own file viaFile::get_offset_location.Corpus gate
Re-harvest of ladybug
src/(1,081 TUs), pre-fix against head:for_initfor_condfor_inc100 tags removed across 31 methods, 0 added, 0 moved. 30 of the 31 are macro loops in
third_party/roaring_bitmap/roaring.c; the 31st is mbedTLS'SHIFT_AND_PLACE. Every decline is a genuinely macro-emitted header; no non-macro loop declines.Test Plan
events.rscovering full headers, barefor (;;), missingcond/ missinginc, comma operators, nested parens incond, a;inside a char literal,whileand range-fornever receiving slot tags, and the three macro shapes: header from a macro (untagged), body from a macro (tagged), header and body from one macro (untagged).LIBCLANG_PATH=/usr/lib/llvm-18/lib cargo test -p ruff_cpp_spo --features libclang: 70 lib tests + 6 example tests pass.cargo clippy -p ruff_cpp_spo --features libclang --all-targets -- -D warningsclean;cargo fmt -p ruff_cpp_spo -- --checkclean.prekcould not run in this environment (the installeduvrejects the repo'sexclude-newer = "P7D"setting and refuses the lockfile under--locked), so the fmt / clippy / test gates above were run directly. CI'sprekrun caught one thing they cannot: thetyposhook rejectedmis-taggedin a doc comment, now reworded.🤖 Generated with Claude Code
https://claude.ai/code/session_011JRoXmYPrE8KubXVrDB71L
Summary by CodeRabbit
New Features
forloops by distinguishing initialization, condition, and increment sections.Bug Fixes
while,do, and range-basedforloops.