fix(readability/casting): noexcept *functions - #377
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a false positive in the C-style cast checker where function pointers with noexcept specifiers were incorrectly flagged as deprecated casts. The fix adds noexcept to the list of keywords that indicate a function declaration rather than a cast.
- Adds
noexceptto the regex pattern that identifies function declarations - Updates the comment to clarify that function pointers are handled
- Adds a test case for function pointers with noexcept specifiers
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cpplint.py | Modified regex pattern to include noexcept as a keyword indicating function declarations and updated comment for clarity |
| cpplint_unittest.py | Added test case for function pointer with noexcept specifier to verify the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lovewave02
left a comment
There was a problem hiding this comment.
Adding noexcept to the generic remainder exemption also hides real casts where noexcept is the C++ operator. On this head, bool safe = (bool) noexcept(f()); produces no readability/casting diagnostic, while current develop reports it and c++ -std=c++11 -fsyntax-only accepts the expression.
Please restrict the exemption to a function declaration or function-pointer context and add regression cases for both the intended declaration and the noexcept(...) operator expression.
without space it seems rare that a cast would directly follow a closing parenthesis and this excludes function pointers, avoiding the noexcept issue entirely without accidentally skipping the noexcept operator as well add unit test for noexcept operator true positive add TODO on refactoring CheckCStyleCast()
7c3a100 to
f44a09a
Compare
📝 WalkthroughWalkthroughUpdated deprecated C-style cast detection to handle pointer expressions involving ChangesCasting detection
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_unittest.py`:
- Line 1113: Update the test case in TestLint to add coverage for the exact bare
noexcept declaration “void (*execute_)(operation_base*) noexcept;” alongside the
existing conditional noexcept(may_throw()) case, preserving the expected lint
result.
In `@cpplint.py`:
- Line 6723: Update the reinterpret_cast detection regex in the relevant cpplint
check to suppress only function-pointer declarator patterns, not every cast
whose opening parenthesis follows a closing parenthesis. Preserve detection of
valid casts such as the conditional cast example, and add a regression test
covering that case.
🪄 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
Run ID: 197d6d91-2f7f-4d37-b4ed-0358bb215a06
📒 Files selected for processing (2)
cpplint.pycpplint_unittest.py
androvonx95
left a comment
There was a problem hiding this comment.
Confirmed this fixes #354 at f44a09a, and lovewave02's noexcept-operator cases still warn correctly ((Type**) noexcept(f()), (bool) noexcept(f())).
Concern: the (?<!\)) lookbehind skips any pointer cast preceded by ), not just function pointers. These warn on develop, silent here:
uintptr_t a = (uintptr_t)(void*)p;
Handle h2 = (Handle)(void*)h;
g(f((Foo)(char*)p));
x = *(Foo)(char*)p;Adding one space ((uintptr_t) (void*)p) restores the warning, so it keys off formatting rather than syntax.
Alternative: keep the original \((\w+\s?\*+\s?)\) pattern, and add this before the remainder check in CheckCStyleCast():
# Parameter lists of function-pointer declarators are not casts, e.g.
# void (*execute_)(operation_base*) noexcept;
if re.search(r"\((?:[^()]*::)?\s*\*\s*\w*\s*\)\s*$", context):
return FalseTested on current develop: fixes #354, keeps both noexcept-operator true positives, keeps all four lines above warning, covers member function pointers (void (Cls::*m)(operation_base*) noexcept;), full suite passes (206 tests).
yangfan-yf-yf
left a comment
There was a problem hiding this comment.
The current fix is still tied to the two parentheses being adjacent. At f44a09a, the exact spelling from #354 is clean, but these equivalent declarations still report Using C-style cast. Use reinterpret_cast<operation_base*>(...) instead:
void (*execute_) (operation_base*) noexcept;
void (*execute_)
(operation_base*) noexcept;
void (*execute_) /* comment */ (operation_base*) noexcept;Whitespace and comments do not change the function-pointer declaration, but they make the (?<!\)) guard stop applying. The existing regression covers only the no-space form, so the test suite remains green despite this gap. Could the suppression use the surrounding declarator context instead of immediate character adjacency, with at least the spaced or multiline form added as a regression?
I ran the focused casting test and the full suite on the exact head (1 passed; 225 passed), and the current GitHub merge ref with develop also passes the full suite (231 passed).
PNHD
left a comment
There was a problem hiding this comment.
Re-reviewed current head f44a09a. I independently confirmed the existing blocker remains: the suppression still depends on immediate parenthesis adjacency, so equivalent spaced/multiline/commented function-pointer declarations can regress while unrelated pointer-cast behavior is affected. No additional inline comments from me; the existing current-head findings cover it.
Fixes #354
this shall be squashed
Summary by CodeRabbit
Bug Fixes
noexcept.Tests
noexceptfunction-pointer syntax.