Repository navigation
fix: handle operators after brace initialization - #474
walidbi200 wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe brace-spacing check now suppresses false positives for nested brace initialization followed by operators. Tests cover arithmetic, comparison, bitwise, shift, and logical operators, plus ChangesBrace spacing false-positive fix
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Suggested reviewers: Merge Risk: ⚪ Minimal · up to The brace-spacing rule avoids the reported false positives while preserving relevant diagnostics, and the supplied regression coverage supports merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
PNHD
left a comment
PNHD
left a comment
There was a problem hiding this comment.
Reviewed head 15836727ece29678e2be254e34d7acaf47008cfb against
862844602d3b30731a36b15965360e576adf7ec9.
The original #373 false positive is fixed, but the new
operator_after_braced_value exception also suppresses valid block-spacing
diagnostics.
At cpplint.py:4748–4751, a word at the end of leading_text is not sufficient
to identify a braced value: it also accepts else, mutable, and noexcept.
For example:
int ElseBlock(int value) {
if (value > 0) {
--value;
} else{
value = 0;
}
++value;
return value;
}
bool MutableLambda() {
return []() mutable{ return 1; } == nullptr;
}
bool NoexceptLambda() {
return []() noexcept{ return 1; } == nullptr;
}All three missing spaces are reported by the base and silently accepted by this
head. The else case also shows that an operator starting a subsequent statement
can trigger the new exception.
I verified this with Python 3.12.14 using the actual base/head cpplint CLI and
--filter=-,+whitespace/braces. A combined file containing these cases plus the
original std::optional{pmr_string{a} + " " + b} shape gives four warnings on the
base and none on the head; the expected result is three warnings. The complete
reproducer also passes g++ -std=c++20 -fsyntax-only with GCC 13.3.
Please preserve block/lambda diagnostics before accepting the operator
continuation, including keyword-ended contexts, and add these negative controls
alongside the initializer regression tests.
The PR's Actions run is green. I did not run the full pytest suite locally because
pytest is unavailable in this environment; the comparison above is an independent
CLI reproduction.
|
I prepared an optional follow-up for the existing block/lambda false-negative review, based on the current head The operator exemption excludes block/lambda specifiers and named lambda trailing-return contexts. Six missing-space cases fail on the current head and pass with the patch; their correctly spaced counterparts and the original 18 initializer/operator cases remain green. A strict C++20 fixture and a comparison through The branch also includes a merge of current develop, preserving both histories. Only the changelog required manual resolution; both entries are retained. The source and tests merged automatically. Local validation: the behavior patch passed 243 tests; the final merged tree passed 253 tests with 96.62% coverage, configured Pylint and Mypy. Pre-commit checks passed on changed files; the all-files conflict hook flags an unchanged README reStructuredText underline while the local merge is staged (the README matches both parents). An independent check also exercised the production path and initializer controls. Prepared with AI assistance on macOS/Python 3.12; remote CI has not run on the follow-up. This is available to take or adapt and retains the existing heuristic parser. |
Fixes #373.
The brace-spacing check already recognizes several contexts where
{}is initialization rather than a block, but it can misclassify a nested braced value when its closing brace is followed by an operator.For example:
This currently reports
whitespace/bracesonpmr_string{a}.This change recognizes operator continuation after a braced value while preserving block-spacing diagnostics such as malformed
requires(T t){ ... }and lambda bodies.Regression coverage includes the original #373 shape, nested braced values followed by arithmetic, bitwise, comparison, shift, and logical operators, plus negative controls for block braces.
Validation:
git diff --check: passed