| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Immunize lines with `&(struct` in them
There was a problem hiding this comment.
This PR fixes a false positive in the trailing semicolon checker that incorrectly flagged compound literal references (address-of expressions with struct compound literals like &(struct name){}) as having unnecessary semicolons. The fix adds a condition to skip the warning when the code contains the pattern of an ampersand followed by a struct compound literal.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpplint.py | Adds check to suppress semicolon warnings for struct compound literal references by detecting & before (struct pattern |
| cpplint_unittest.py | Adds test case verifying &(struct mount_attr){} doesn't trigger false positive |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
let's see if the \s makes the tests fail Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
I reproduced the original issue and confirmed that the single-line struct case is fixed. The existing targeted test passes, the full functional suite passes (225 tests without the pre-existing coverage gate), and pylint, mypy, pre-commit, and the remote CI checks pass.
Requesting changes for the following issues:
cpplint.py:5087 only recognizes struct/union when the keyword is on the same physical line as the opening parenthesis. C permits whitespace, including newlines, in the compound-literal type name. On this head, the following valid code still produces You don't need a ; after a } [readability/braces] [4]:
struct S *s = &(
struct S
){};The same false positive remains for union U. Please make the detection work across the expression returned by ReverseCloseExpression and add multiline regression coverage.
cpplint_unittest.py:3065 exercises only struct, although the implementation explicitly handles both struct and union. Please add a union compound-literal reference test and negative boundary cases such as &(structure){} and &(struct_name){}.
CONTRIBUTING.rst requires every PR to add a summary to CHANGELOG.rst, but this PR does not modify the changelog.
For context, running pytest with the repository's coverage configuration executes all 225 tests successfully but exits on the existing 89.36% coverage result versus the 90% threshold. I reproduced the identical result on base SHA 6734a6d, so I am not treating that as a regression from this PR.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the re-review request! I took another look, but the PR is still on the same commit (a44637e) as my previous review, so the points I raised there are still outstanding: multiline struct/union handling and regression tests, the additional union/boundary cases, and the CHANGELOG.rst entry.
Once those updates are pushed, feel free to re-request my review and I'll be happy to take another look.
Sorry, something went wrong.
There was a problem hiding this comment.
One valid C case is still uncovered. On this head, both of these still emit You don't need a ; after a }:
const struct S *s = &(const struct S){};
volatile union U *u = &(volatile union U){};cc -std=c11 -fsyntax-only accepts both. The new matcher only accepts a type name that begins directly with struct or union. Please handle type qualifiers in the compound-literal type name and add const/volatile regression cases.
Sorry, something went wrong.
|
I checked out a44637e and reproduced independently:
Agree with the earlier review points: qualify detection across ReverseCloseExpression (and allow const/volatile), add union + qualifier + multiline tests, and add a CHANGELOG.rst TBA entry per CONTRIBUTING.rst. |
Sorry, something went wrong.
There was a problem hiding this comment.
There is another single-line compound-literal case that the new guard does not cover. The required semicolon is not specific to struct or union specifiers: C compound literals can use scalar and typedef type names as well. On a44637e, both of these still emit You don't need a ; after a }:
int *value = &(int){0};
typedef struct S { int x; } S;
S *value = &(S){0};Both semicolons are required. The typedef form has the same address-of compound-literal shape as the reported case; only the spelling of the type name differs. Please make the exemption recognize the address-of compound-literal form rather than only a struct/union prefix, and add scalar and typedef regression cases. The full test suite passes (225 passed), so these cases need focused coverage.
Sorry, something went wrong.
There was a problem hiding this comment.
Re-reviewed current head a44637e. I independently confirmed the existing current-head blockers remain: valid compound literals outside the narrow single-line struct/union form are still mishandled, and the required CHANGELOG entry is still missing. I have no new inline findings to add beyond the existing reviews.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Immunize lines with &(struct in them
Should fix #201