GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — fix(runtime/explicit): skip deleted constructors by yangfan-yf-yf · Pull Request #450 · cpplint/cpplint Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDeleted single-argument constructors are detected across multiline declarations and excluded from the ChangesDeleted constructor warning handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpplint.py`:
- Around line 3931-3940: Update the constructor detection and related state
handling around explicit_constructor_match to recognize suffixes such as
noexcept before = delete;, and preserve deletion state when the declaration
continues onto subsequent lines before the deleted marker. Ensure deleted
constructors do not emit runtime/explicit, and add regression coverage for
same-line suffixes and multiline declarations.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83d1e3b8-daa3-47fd-8b0c-3ebc77e753e5
CHANGELOG.rstcpplint.pycpplint_unittest.py
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Addressed in The constructor matcher now checks the declaration suffix after the closing Added regressions for both forms. Validation: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpplint.py`:
- Around line 3942-3952: Update the constructor suffix parsing around the
deleted-constructor detection to track balanced parentheses, braces, and other
delimiters so braces inside nested noexcept(...) expressions do not terminate
the suffix early. Only stop at declaration-level terminators, then ensure the
existing is_deleted regex recognizes valid nested noexcept forms and add a
regression case covering a deleted constructor such as Foo(int)
noexcept(noexcept(T{})) = delete;.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0273c0de-36f6-4978-8122-c43d9c96a9ea
cpplint.pycpplint_unittest.py
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Addressed the nested-expression case in The suffix reader now tracks parentheses, brackets, and braces, and only treats Added that regression alongside the existing one-line and multiline cases. Validation: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
One gap: the template <class T> Foo(T v) requires Integral<T> = delete; // C++20, trailing requires-clause Foo(int f) throw() = delete; // valid through C++17 Foo(int f) = delete("use Bar instead"); // C++26, P2573 Each compiles under Since is_deleted = bool( re.search(r"=\s*delete\s*(?:\(.*\))?\s*;\s*$", constructor_suffix, re.DOTALL) ) Tested locally on your branch: 206 tests pass, and no over-suppression — inline body, member-init body, declaration followed by a deleted overload, two constructors on one line, macro suffix, and a trailing |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed at 962e80f. The _GetConstructorSuffix() approach is sound and the #386 case, the noexcept forms, and the split-line form are all clean.
Not approving yet: as noted in my comment above, the is_deleted regex enumerates allowed suffixes, so a trailing requires-clause, throw(), and C++26 = delete("reason") still emit runtime/explicit. Anchoring the match to the end of the extracted suffix fixes all three, passes the full suite, and doesn't over-suppress.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Addressed in
Added regressions for all three forms. Validation: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
cpplint_unittest.py (1)
1668-1689: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd positive controls for non-deleted suffixes.
These cases only verify suppression. Add corresponding non-deleted declarations such as
Foo(int f) requires Integral<int>;,Foo(int f) throw();, andFoo(int f) noexcept;expectingruntime/explicit; otherwise a suffix-parsing regression that skips these declarations could pass the tests.🤖 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 1668 - 1689, Add positive-control cases to the TestMultiLineLint coverage for non-deleted declarations: require `runtime/explicit` for declarations using `requires Integral<int>`, `throw()`, and `noexcept` suffixes. Keep the existing deleted-declaration suppression cases unchanged and place the new assertions alongside them.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpplint_unittest.py`:
- Around line 1668-1689: Add positive-control cases to the TestMultiLineLint
coverage for non-deleted declarations: require `runtime/explicit` for
declarations using `requires Integral<int>`, `throw()`, and `noexcept` suffixes.
Keep the existing deleted-declaration suppression cases unchanged and place the
new assertions alongside them.
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb5fe3b1-688f-4ab5-ab05-e32c4a89529a
cpplint.pycpplint_unittest.py
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Rechecked at bea44fe. The three previously reported forms are now handled correctly, while equivalent non-deleted constructors still emit runtime/explicit. The full unit suite passes (206 tests). My concern is resolved — LGTM.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
bea44fe to
453f31e
Compare
|
Rebased onto the current Validation after the rebase: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
cpplint_unittest.py (1)
1666-1673: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a negative counterpart for the
requiresclause case.This test confirms that a templated single-parameter constructor with a trailing
requiresclause and= deleteproduces no warning. Add a companion test for the same declaration without= delete, to confirmruntime/explicitstill fires when the constructor is not deleted. This guards against a suffix scan that is too permissive and silently swallows therequiresclause together with legitimate declarations that should still warn.Suggested additional test
self.TestMultiLineLint( """ class Foo { template <class T> Foo(T value) requires Integral<T>; };""", "Single-parameter constructors should be marked explicit. [runtime/explicit] [4]", )🤖 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 1666 - 1673, Add a companion TestMultiLineLint case beside the existing deleted-constructor test, using the same templated constructor with its trailing requires clause but omitting = delete; assert the expected runtime/explicit warning so suffix handling does not suppress valid diagnostics.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpplint_unittest.py`:
- Around line 1666-1673: Add a companion TestMultiLineLint case beside the
existing deleted-constructor test, using the same templated constructor with its
trailing requires clause but omitting = delete; assert the expected
runtime/explicit warning so suffix handling does not suppress valid diagnostics.
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a19f7e1-6374-42e4-ac4b-899803ed2435
CHANGELOG.rstcpplint.pycpplint_unittest.py
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed current head c7d895c.
The deleted-constructor detection is bounded by the extracted top-level declaration suffix and covers the relevant noexcept, trailing requires, throw(), diagnostic = delete(...), and split-line forms while preserving the non-deleted requires positive control.
The final commit only strengthens that positive-control coverage, and hosted Test is green on this head. LGTM.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed current head c7d895c and found one additional deleted-constructor edge case in the suffix scanner.
A valid C++20 constrained deleted constructor can use a requires-expression with a top-level braced requirement, for example:
template <class T>
Foo(T) requires requires { typename T::type; } = delete;
_GetConstructorSuffix() currently treats a { seen at zero paren/bracket/brace depth as the constructor body and returns immediately. It therefore truncates this suffix before = delete;, leaving is_deleted false and allowing a spurious runtime/explicit diagnostic.
I verified the declaration with g++ -std=c++20 -pedantic-errors -fsyntax-only. This differs from the covered noexcept(noexcept(T{})) case because that brace occurs under nonzero parenthesis depth.
Please distinguish a braced requires-expression from the actual constructor body and add regression coverage for this form.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Addressed in b833ee8. The suffix scanner now distinguishes the initial requires-clause from subsequent requires-expressions, keeps scanning through their requirement bodies, and stops at the actual constructor body. The regressions cover the reported form, optional requires-expression parameters, multiline requirements, compound requirements, and conjunctions. Matching non-deleted declarations still warn, and body-boundary checks prevent scanning into a later deleted constructor. The C++20 declarations pass The full pytest suite passes (233 tests, 96.40% coverage), as do Pylint and the configured pre-commit hooks for the changed files. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Reviewed current head 8e213e7. I reproduced the previously reported braced requires-expression failure on the older head and confirmed the current implementation now scans through the requirement body, recognizes the trailing = delete;, and still warns for equivalent non-deleted constructors and constructors with real bodies.
I also exercised additional constrained constructors, nested noexcept/requires expressions, multiline requirements, conjunctions/disjunctions, body-boundary cases, and potential false-suppression cases without finding a valid-code regression.
Focused tests and the full suite pass (234 tests, 96.40% coverage), along with pylint, mypy, pre-commit, and git diff --check. No remaining blocker from me on this head. LGTM.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
AI-assisted review of 8e213e7. I found a remaining C++20 false positive in _GetConstructorSuffix() at cpplint.py:3806-3807: a brace inside a concept non-type template argument is treated as the constructor body.
template <auto V> concept NonNegative = (V >= 0); class Foo { public: template <class T> Foo(T value) requires NonNegative<T{}> = delete; };
This passes clang++ -std=c++20 -pedantic-errors -fsyntax-only, but the head still emits runtime/explicit. The collected suffix is only requires NonNegative<T, so = delete; is never reached. NonNegative<[] { return 1; }()> reproduces the same issue.
Could these template-argument braces be preserved, with deleted/non-deleted controls and the existing real-body boundary tests retained? Plain deletion and a simple constraint are fixed by this PR; the two cases above remain false positives on both base and head, so this is incomplete coverage of the fix, not a new regression. The unchanged full suite passes locally: 234 tests, 96.40% coverage, Python 3.12.14/macOS arm64.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
For the braced template-argument cases above, I prepared an optional follow-up on The added controls cover Known limit: the shared matcher still does not recognize unparenthesized NTTP comparisons such as |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
At least 1 approving review is required to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
False positive for runtime/explicit on deleted constructors
There was an error while loading. Please reload this page.