fix: handle templated lambdas in semicolon check - #454
Conversation
📝 WalkthroughWalkthroughThe change updates trailing-semicolon detection for C++20 templated lambdas, including multiline ChangesTemplated Lambda Detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CheckTrailingSemicolon
participant MatchLambdaCapture
participant MatchLambdaRequiresClause
CheckTrailingSemicolon->>MatchLambdaCapture: match lambda capture and template boundary
MatchLambdaCapture-->>CheckTrailingSemicolon: return capture match
CheckTrailingSemicolon->>MatchLambdaRequiresClause: inspect multiline requires clause
MatchLambdaRequiresClause-->>CheckTrailingSemicolon: return preceding capture match
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
androvonx95
left a comment
androvonx95
left a comment
There was a problem hiding this comment.
Reviewed at bce7f59 against current develop.
Fixes the #385 case and the harder valid C++20 forms that a same-line ]<...> check misses — multiline template parameter lists, ]<T> with ( on the next line, and trailing requires-clauses. I compiled those with g++ -std=c++20 -pedantic-errors -fsyntax-only and confirmed they stay clean here while ordinary templated/constrained functions and operator[] (including multiline) still warn. Full unit suite: 206 passed.
This conflicts with #458; I’d take this one — it’s the more complete coverage for the same issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpplint_unittest.py (1)
5787-5793: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the diagnostic for
foo.cc.Line 5792 counts
bar_header_error, but this invocation lintstest/foo.cc. The assertion passes even if includingfoo.hppincorrectly reports thatfoo.ccomitted its related header.Proposed fix
- assert error_collector.Results().count(bar_header_error) == 0 + assert error_collector.Results().count(foo_header_error) == 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpplint_unittest.py` around lines 5787 - 5793, Update the assertion in the multiple-same-basename test to count the diagnostic associated with foo.cc rather than bar_header_error, while preserving the expectation that including foo.hpp produces no related-header error for foo.cc. Use the existing diagnostic symbol or construction for foo.cc from the surrounding test setup.
🧹 Nitpick comments (1)
cpplint_unittest.py (1)
3169-3201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a trailing lambda
requiresclause.The new tests only cover a
requiresclause before the lambda parameter list. C++20 also permits a clause after the parameter list. A matcher regression in that form will pass this suite.Proposed test
+ self.TestMultiLineLint( + "auto identity = []<typename T>(T&& t)\n" + " requires std::integral<T> &&\n" + " std::copyable<T>\n" + "{\n" + " return t;\n" + "};\n", + "", + )Verify this form with the project C++20 compiler.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpplint_unittest.py` around lines 3169 - 3201, Add a TestMultiLineLint case alongside the existing generic lambda requires-clause cases covering a trailing requires clause after the lambda parameter list, including a multiline constraint if appropriate. Use valid C++20 syntax and verify the test with the project’s C++20 compiler, expecting no lint errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpplint_unittest.py`:
- Around line 5787-5793: Update the assertion in the multiple-same-basename test
to count the diagnostic associated with foo.cc rather than bar_header_error,
while preserving the expectation that including foo.hpp produces no
related-header error for foo.cc. Use the existing diagnostic symbol or
construction for foo.cc from the surrounding test setup.
---
Nitpick comments:
In `@cpplint_unittest.py`:
- Around line 3169-3201: Add a TestMultiLineLint case alongside the existing
generic lambda requires-clause cases covering a trailing requires clause after
the lambda parameter list, including a multiline constraint if appropriate. Use
valid C++20 syntax and verify the test with the project’s C++20 compiler,
expecting no lint errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6381b6bc-67e3-421c-ac55-69ec440f368d
📒 Files selected for processing (3)
CHANGELOG.rstcpplint.pycpplint_unittest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.rst
- cpplint.py
|
Thanks for the review! I resolved the conflict with current develop in fee646d, preserving both changelog entries. All 231 tests and repository checks pass locally. Could you please take another look when you have time? |
androvonx95
left a comment
androvonx95
left a comment
There was a problem hiding this comment.
Rechecked at fee646d. Merge with current develop looks clean — both changelog entries are preserved, and the templated-lambda logic is unchanged from bcf7f59.
Re-ran the earlier cases against this head: #385, multiline template params, ]<T>n(, and requires-clauses stay clean; templated/constrained functions and multiline operator[] still warn. Full suite: 206 passed. Still LGTM.
yangfan-yf-yf
left a comment
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
Reviewed at fee646d.
The matcher keeps ordinary constrained functions and multiline operator[] declarations in the trailing-semicolon path while recognizing the C++20 templated-lambda forms. I also checked a trailing requires-clause lambda and confirmed it stays clean under the repository's brace-style rule.
Verified locally:
python -m pytest --no-cov -q
231 passed
git diff --check upstream/develop...HEAD
Passed
LGTM.
3 participants
Fixes #385.
The trailing-semicolon check now recognizes C++20 templated lambdas,
including multiline template parameter lists and requires-clauses.
The existing warnings for operator[] and ordinary constrained function
definitions remain unchanged.
Tests:
Summary by CodeRabbit
Bug Fixes
requiresclauses.Tests
Documentation