Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #10305 & #10699: FP unusedStructMember with aggregate initialization #6797

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ludviggunne
Copy link
Contributor

Compares number of non-static members with number of entries in braced initializer.

@ludviggunne ludviggunne changed the title fix #10305 fix #10305: FP unusedStructMember with aggregate initialization Sep 12, 2024
lib/checkunusedvar.cpp Outdated Show resolved Hide resolved
@olabetskyi
Copy link
Collaborator

10699 Can be added to title as it's pretty much the same.

@ludviggunne ludviggunne changed the title fix #10305: FP unusedStructMember with aggregate initialization fix #10305 & #10699: FP unusedStructMember with aggregate initialization Sep 12, 2024
@ludviggunne ludviggunne marked this pull request as ready for review September 15, 2024 07:09
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@@ -1534,7 +1534,14 @@ void CheckUnusedVar::checkStructMemberUsage()
if (bailout)
continue;

// Keep track of number of non-static members encountered so far
int nummembs = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming.. I like someName use a capital letter for first letter in each word. And I prefer full name. "membs" is much harder to read than "members" and you only save 2 characters.

@@ -1544,6 +1551,22 @@ void CheckUnusedVar::checkStructMemberUsage()
// Check if the struct member variable is used anywhere in the file
bool use = false;
for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
if (Token::Match(tok, "%name% {") && !Token::Match(tok->previous(), "class|struct") && tok->str() == scope.className && tok->linkAt(1)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate that you take nothing for granted but it is redundant to check if a { has a link. Cppcheck will output a syntax error if there is no corresponding } in the source code.

checkStructMemberUsage("struct S {\n"
" const int* p{};\n"
"};\n"
"struct C {\n"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the point to have struct C. a function is less code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I tried to imititate the code in the ticket, but I agree it's unnecessary.

@danmar
Copy link
Owner

danmar commented Sep 28, 2024

In my humble opinion here is a better test case. There should be no "unusedStructMember" warning here:

struct S {
    int a;
};

S foo() {
    return S{0};
}

@ludviggunne ludviggunne marked this pull request as draft September 28, 2024 11:31
@ludviggunne
Copy link
Contributor Author

I realize know that this doesn't actually solve the main in example in #10305 (only the simpler one in the comments), since it relies on the name of the struct appearing before the braced initalizer. So this will require some more work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants