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
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.
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