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(readability/casting): ignore calling-convention function pointers by androvonx95 · Pull Request #448 · cpplint/cpplint Conversation
|
📝 WalkthroughWalkthroughThe casting checks now avoid reporting function-pointer typedef and alias declarations with calling-convention macros before ChangesCasting false-positive fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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.
cpplint.py (2)
6689-6689: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider supporting arrays of function pointers.
Using
\w*for the function pointer name restricts it to word characters. This means the regex will fail to match arrays of function pointers, such as(CALLCONV* FuncArray[3])(void), because\w*does not match the[and]characters.To seamlessly handle these valid cases while avoiding spaces and parentheses, consider replacing
\w*with[^() ]*. This mirrors the logic used in the adjacent pointer-to-member regex (line 6688).🛠 Proposed fix
- or re.match(r"\((?:\w+\s*)?\*\s*\w*\)\s*\(", matched_funcptr) + or re.match(r"\((?:\w+\s*)?\*\s*[^() ]*\)\s*\(", matched_funcptr)🤖 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.py` at line 6689, Update the function-pointer regex alternative near the adjacent pointer-to-member pattern to allow array declarators in names such as FuncArray[3]. Replace the restrictive word-character name match with a character class that excludes parentheses and spaces, preserving the existing surrounding function-pointer matching behavior.
6831-6836: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider edge cases like function parameters and trailing return types.
The
typedefandusingcheck perfectly addresses the alias cases mentioned in the issue. However, be aware that this same false positive can still occur if the function-pointer type is used directly as a function parameter or trailing return type, for example:
void Register(int32_t(CALLCONV *)(void));Since
(CALLCONV *)is followed by(void), it will still be flagged as a C-style cast here because the line lackstypedeforusing.While fully addressing this might be out of scope for this specific PR, it's worth keeping in mind or adding a TODO for future improvement.
🤖 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.py` around lines 6831 - 6836, Extend the false-positive guard near the existing typedef/using check to recognize function-pointer type syntax used directly in function parameters or trailing return types, including patterns such as `(CALLCONV *)` followed by `(void)`. Preserve the current alias handling and add a focused TODO if complete support is out of scope.
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.py`:
- Line 6689: Update the function-pointer regex alternative near the adjacent
pointer-to-member pattern to allow array declarators in names such as
FuncArray[3]. Replace the restrictive word-character name match with a character
class that excludes parentheses and spaces, preserving the existing surrounding
function-pointer matching behavior.
- Around line 6831-6836: Extend the false-positive guard near the existing
typedef/using check to recognize function-pointer type syntax used directly in
function parameters or trailing return types, including patterns such as
`(CALLCONV *)` followed by `(void)`. Preserve the current alias handling and add
a focused TODO if complete support is out of scope.
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4f26bba-62e4-47aa-828a-ae55db35b0dd
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.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
One declaration form in the stated scope is still flagged:
typedef int32_t(CALLCONV* FuncArray[3])(void);
On the current head it emits Using deprecated casting style. Use static_cast<int32_t>(...) instead. The new function-pointer alternative uses \w* for the declarator name, so it stops before [3]; the adjacent member-function-pointer pattern already accepts array declarators. Please broaden that declarator portion and add a regression case for this typedef form.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Below: This branch has conflicts that must be resolved |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…f/using Broaden the declarator part of the function-pointer pattern so that array declarators such as `typedef int32_t(CALLCONV* FuncArray[3])(void);` are recognized instead of being reported as a deprecated cast.
|
I'm going to see if I can port changelog-addition to towncrier. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
For slow moving repos, Towncrier always seems to be more process complexity than rebasing but do what you think is best. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I also reran the relevant tests plus the full suite, and everything passes. I resolved the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Re: Towncrier... |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
FTR: A rebase isn't needed; we have a workflow that automatically adds a changenote to dependabot PRs, but in the case of the PR you've tagged, it was misconfigured (missing a required permission) as part of a recent spring cleaning in that project. |
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
cpplint_unittest.py (1)
5761-5766: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the result of the
foo.cccase.The test reuses
error_collectorfrom the previousBar.ccchecks and assertsbar_header_error. A new error from thefoo.cc/foo.hppcase cannot fail this assertion. Reseterror_collectorbeforeProcessFileDataand assertfoo_header_errorfor this case.Proposed fix
+ error_collector = ErrorCollector(self.assertTrue) cpplint.ProcessFileData( "test/foo.cc", "cc", [r'`#include` "foo.hpp"', ""], error_collector, ) - 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 5761 - 5766, Update the foo.cc test case around ProcessFileData to reset error_collector before processing, then assert the collected results against foo_header_error rather than bar_header_error, preserving the existing Bar.cc assertions.
cpplint_unittest.py (1)
1119-1133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a positive control for the new suppression.
These tests verify only that function-pointer declarations produce no warning. Add a nearby case that must still report a real cast with similar tokens. This will detect overmatching in the new
typedefandusingsuppression.🤖 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 1119 - 1133, Add a nearby positive-control test in the relevant cpplint unittest alongside the existing function-pointer cases, using similar calling-convention/function-pointer tokens but an actual C-style cast that must produce the expected warning. Keep the current no-warning assertions unchanged and use TestLint’s expected-output argument to verify the suppression does not overmatch.
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 5761-5766: Update the foo.cc test case around ProcessFileData to
reset error_collector before processing, then assert the collected results
against foo_header_error rather than bar_header_error, preserving the existing
Bar.cc assertions.
---
Nitpick comments:
In `@cpplint_unittest.py`:
- Around line 1119-1133: Add a nearby positive-control test in the relevant
cpplint unittest alongside the existing function-pointer cases, using similar
calling-convention/function-pointer tokens but an actual C-style cast that must
produce the expected warning. Keep the current no-warning assertions unchanged
and use TestLint’s expected-output argument to verify the suppression does not
overmatch.
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 682e69f1-af3c-4a0c-90ba-f742bc819288
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.
Rechecked at 62c0094.
The function-pointer declarator pattern now accepts array declarators, so the previously reported typedef int32_t(CALLCONV* FuncArray[3])(void); case is clean. I also verified a using alias with the calling-convention form stays clean while an ordinary C-style cast still reports readability/casting.
Verified locally:
python -m pytest --no-cov cpplint_unittest.py -k 'testDeprecatedCast or testCStyleCast' -q
2 passed
python -m pytest --no-cov -q
231 passed
git diff --check upstream/develop...HEAD
Passed
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 62c0094. The previously reported array-declarator case is fixed, the calling-convention typedef/alias cases are covered, and ordinary C-style casts remain diagnosed. I found no remaining blocker 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.
Reviewed current head 62c0094. I reproduced the original calling-convention function-pointer false positive on the base commit and the earlier array-declarator failure, and confirmed both are fixed on this head.
I also exercised typedef and using forms, array declarators, calling-convention spacing variants, ordinary function pointers, pointer-to-member cases, and real C-style cast controls. The broadened function-pointer matching did not suppress the ordinary casts I tested.
The focused casting tests and full suite pass locally, along with pylint, mypy, pre-commit, and git diff --check. I found no regression introduced by this change and no remaining blocker 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 62c0094. The calling-convention typedef/alias and array-declarator fixes work, but the new exemption at cpplint.py:6811-6812 also hides real casts inside type aliases:
void* p = nullptr; using Ptr = decltype((int*)(p)); typedef decltype((int*)(p)) Ptr2;
Both aliases pass clang++ -std=c++17 -pedantic-errors -fsyntax-only. Each produces readability/casting on the base, but no casting diagnostic on this head. These are pointer casts, not function-pointer declarators.
This concretely exercises the earlier positive-control concern: typedef/using anywhere on the line plus a parenthesized operand is too broad. Please restrict the exemption to the declarator context and add these real-cast controls. Ordinary casts still report, and the two existing focused casting tests pass locally (Python 3.12/macOS).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I prepared a follow-up for the alias-cast regression on the unchanged head It limits the exemption to the top level of the declared type, preserving casts inside Local validation on Python 3.12/macOS: 232 tests passed, 96.44% coverage; Pylint, Mypy, and all configured pre-commit hooks passed. This is still a focused same-line heuristic, not a complete declarator parser. AI-assisted implementation and review; the reported checks were executed. |
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.
[readability/casting] false positive with typedef
There was an error while loading. Please reload this page.