fix(runtime/explicit): skip deleted constructors by yangfan-yf-yf · Pull Request #450 · cpplint/cpplint · GitHub

Purpose

Fixes #386. Deleted single-argument constructors currently trigger runtime/explicit even though they cannot be called implicitly.

Rationale

The explicit-constructor check classifies the parameter list but does not inspect the declaration suffix. It now recognizes a trailing = delete; and skips the warning only for that declaration form. Non-deleted single-argument constructors retain the existing check.

How did you test?

Added a regression case for Foo(int f) = delete; alongside the existing missing-explicit cases.
Ran python -m pytest --no-cov cpplint_unittest.py -k testExplicitSingleArgumentConstructors -vv: 1 passed.
Ran python -m pytest --no-cov -q: 231 passed.
Ran python -m pylint cpplint.py.
Ran python -m pre_commit run --files CHANGELOG.rst cpplint.py cpplint_unittest.py.
Ran git diff --check.

How to Verify

Run python -m pytest --no-cov cpplint_unittest.py -k testExplicitSingleArgumentConstructors -vv.
Confirm the deleted constructor produces no runtime/explicit message while the existing ordinary constructor case still does.

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Deleted single-argument constructors are detected across multiline declarations and excluded from the runtime/explicit warning. Regression tests cover deletion syntax variants, and the changelog records the fix.

Changes

Deleted constructor warning handling

Layer / File(s)	Summary
Constructor detection and validation cpplint.py, cpplint_unittest.py, CHANGELOG.rst	The lint check scans constructor suffixes, identifies deleted single-argument constructors, suppresses the runtime/explicit warning, tests multiple declaration formats, and records the change.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes suppress runtime/explicit warnings for deleted single-argument constructors while preserving checks for non-deleted constructors, satisfying issue #386.
Out of Scope Changes check	✅ Passed	The implementation, regression tests, and changelog entry directly support the linked issue and stated pull request objectives.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the fix for skipping runtime/explicit warnings on deleted constructors.

✨ Finishing Touches 🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Actionable comments posted: 1

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

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.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info ⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 83d1e3b8-daa3-47fd-8b0c-3ebc77e753e5

📥 Commits

Reviewing files that changed from the base of the PR and between 95551c6 and eb3031a.

📒 Files selected for processing (3)

CHANGELOG.rst
cpplint.py
cpplint_unittest.py

Addressed in 14f3d58.

The constructor matcher now checks the declaration suffix after the closing ) for = delete;, allowing an optional noexcept / noexcept(...) suffix and continuing across following cleansed lines until the declaration terminates. This covers both Foo(int) noexcept = delete; and a newline before = delete; without changing the existing argument analysis.

Added regressions for both forms.

Validation:

python -m pytest cpplint_unittest.py -q --no-cov
206 passed

python -m pre_commit run --files cpplint.py cpplint_unittest.py
Passed

git diff --check
Passed

Actionable comments posted: 1

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

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

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info ⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0273c0de-36f6-4978-8122-c43d9c96a9ea

📥 Commits

Reviewing files that changed from the base of the PR and between eb3031a and 14f3d58.

📒 Files selected for processing (2)

cpplint.py
cpplint_unittest.py

🚧 Files skipped from review as they are similar to previous changes (1)

cpplint_unittest.py

Addressed the nested-expression case in 962e80f.

The suffix reader now tracks parentheses, brackets, and braces, and only treats ; or { as a terminator at declaration depth. A { inside noexcept(...) no longer ends the scan, so Foo(int) noexcept(noexcept(T{})) = delete; is recognized as deleted.

Added that regression alongside the existing one-line and multiline cases.

Validation:

python -m pytest cpplint_unittest.py -q --no-cov
206 passed

python -m pre_commit run --files cpplint.py cpplint_unittest.py
Passed

git diff --check
Passed

_GetConstructorSuffix() is the right shape, and I confirmed #386's case plus the noexcept and split-line forms are clean on 962e80f (the leading requires-clause form works too).

One gap: the is_deleted regex (cpplint.py, ~line 3982) enumerates allowed suffixes, so these valid declarations still emit runtime/explicit:

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 g++ -pedantic-errors -fsyntax-only at the relevant standard.

Since _GetConstructorSuffix() already stops at the top-level ; or {, the suffix either ends in = delete; or it isn't deleted, so checking the end covers all three without enumerating forms:

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 // = delete comment each still warn exactly once. Worth adding a regression for the trailing requires-clause.

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.

Addressed in bea44fe.

is_deleted now searches for a deleted marker at the end of the suffix collected by _GetConstructorSuffix(), rather than enumerating permitted suffix forms. This covers a trailing requires clause, throw(), and the diagnostic form of = delete(...) while preserving the existing declaration-level terminator handling.

Added regressions for all three forms.

Validation:

.venv\Scripts\python.exe -m pytest cpplint_unittest.py -q --no-cov
206 passed

.venv\Scripts\python.exe -m pre_commit run --files cpplint.py cpplint_unittest.py
Passed

git diff --check
Passed

🧹 Nitpick comments (1)

cpplint_unittest.py (1)
1668-1689: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add 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();, and Foo(int f) noexcept; expecting runtime/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.

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

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.

ℹ️ Review info ⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fb5fe3b1-688f-4ab5-ab05-e32c4a89529a

📥 Commits

Reviewing files that changed from the base of the PR and between 962e80f and bea44fe.

📒 Files selected for processing (2)

cpplint.py
cpplint_unittest.py

🚧 Files skipped from review as they are similar to previous changes (1)

cpplint.py

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.

Fixes cpplint#386

Rebased onto the current develop branch and resolved the changelog conflict by retaining both unrelated entries.

Validation after the rebase:

python -m pytest --no-cov cpplint_unittest.py -k testExplicitSingleArgumentConstructors -q
1 passed

python -m pytest --no-cov -q
231 passed

git diff --check upstream/develop...HEAD
Passed

🧹 Nitpick comments (1)

cpplint_unittest.py (1)
1666-1673: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Add a negative counterpart for the requires clause case.

This test confirms that a templated single-parameter constructor with a trailing requires clause and = delete produces no warning. Add a companion test for the same declaration without = delete, to confirm runtime/explicit still fires when the constructor is not deleted. This guards against a suffix scan that is too permissive and silently swallows the requires clause 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.

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

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.

ℹ️ Review info ⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a19f7e1-6374-42e4-ac4b-899803ed2435

📥 Commits

Reviewing files that changed from the base of the PR and between bea44fe and 453f31e.

📒 Files selected for processing (3)

CHANGELOG.rst
cpplint.py
cpplint_unittest.py

🚧 Files skipped from review as they are similar to previous changes (2)

CHANGELOG.rst
cpplint.py

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.

yangfan-yf-yf marked this pull request as ready for review July 25, 2026 08:13

coderabbitai Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread cpplint.py Outdated

coderabbitai Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread cpplint.py Outdated

androvonx95 reviewed Jul 25, 2026

View reviewed changes

coderabbitai Bot reviewed Jul 25, 2026

View reviewed changes

androvonx95 approved these changes Jul 26, 2026

View reviewed changes

yangfan-yf-yf added 4 commits August 4, 2026 18:58

fix(runtime/explicit): skip deleted constructors

00a4e14

Fixes cpplint#386

fix: detect deleted constructor suffixes

fb92da6

fix: handle nested deleted constructor suffixes

01b808d

fix: recognize additional deleted constructor suffixes

453f31e

yangfan-yf-yf force-pushed the agent/skip-explicit-check-deleted-constructors branch from bea44fe to 453f31e Compare August 4, 2026 10:59

coderabbitai Bot reviewed Aug 4, 2026

View reviewed changes

test: cover non-deleted requires constructor

c7d895c

PNHD approved these changes Aug 17, 2026

View reviewed changes

fix(runtime/explicit): skip deleted constructors by yangfan-yf-yf · Pull Request #450 · cpplint/cpplint · GitHub

Uh oh!

fix(runtime/explicit): skip deleted constructors - #450

Conversation

yangfan-yf-yf commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Rationale

How did you test?

How to Verify

Uh oh!

coderabbitai Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yangfan-yf-yf commented Jul 25, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yangfan-yf-yf commented Jul 25, 2026

Uh oh!

androvonx95 commented Jul 25, 2026

Uh oh!

androvonx95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangfan-yf-yf commented Jul 25, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

androvonx95 left a comment

Choose a reason for hiding this comment

Uh oh!

yangfan-yf-yf commented Aug 4, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

PNHD left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yangfan-yf-yf commented Jul 25, 2026 •
edited
Loading

coderabbitai Bot commented Jul 25, 2026 •
edited
Loading

androvonx95 left a comment •
edited
Loading