Skip to content

Suggested implementation for rule 8.2#3169

Merged
danmar merged 10 commits intocppcheck-opensource:mainfrom
larseven:rule_8_2
Mar 25, 2021
Merged

Suggested implementation for rule 8.2#3169
danmar merged 10 commits intocppcheck-opensource:mainfrom
larseven:rule_8_2

Conversation

@larseven
Copy link
Copy Markdown
Contributor

Would like some early feedback on a strategy for supporting rule 8.2.

@amai2012 amai2012 requested a review from danmar March 22, 2021 12:45
Comment thread addons/misra.py Outdated

# Zero arguments should be in form ( void )
if (len(func.argument) == 0):
zeroCallTokens = getFollowingRawTokens(rawTokens, func.tokenDef, 3)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's a mistake to remove the "void". I wonder if we can just remove the Tokenizer::simplifyParameterVoid. Could you check if that would confuse the symboldatabase for some C/C++ code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed Tokenizer::simplifyParameterVoid in my latest commit and updated the tests accordingly. This made it possible to check this case without using the raw tokens.

Comment thread addons/misra.py
if len(rawTokensFollowingPtr) != 3:
continue

# Compliant: returnType (*ptrName) ( ArgType )
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have the feeling that the long term goal would be that the tokenlist has all this information.. but maybe for now it will be easier that you check the rawtokens like this..

Copy link
Copy Markdown
Collaborator

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

yes looks pretty good. however as a long term goal I would prefer if you didn't have to go to the raw tokens..

Comment thread test/testsizeof.cpp
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

all other test cases seem good to me.. I just wonder what happened here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the expected text output here now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do you know why we get such strange warning. I have not debugged it but I don't spontanously see where *) comes from.

Copy link
Copy Markdown
Contributor Author

@larseven larseven Mar 23, 2021

Choose a reason for hiding this comment

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

I think that in CheckSizeof::sizeofVoid (line 407) we could check not only for Token::simpleMatch(tok, "sizeof ( )" but also Token::simpleMatch(tok, "sizeof (void)". Then it will report issue as sizeofVoidError instead of sizeofDereferencedVoidPointerError.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a fix for this now. Verified it locally.

Comment thread lib/checksizeof.cpp Outdated

for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
if (Token::simpleMatch(tok, "sizeof ( )")) { // "sizeof(void)" gets simplified to sizeof ( )
if (Token::simpleMatch(tok, "sizeof ( )") || Token::simpleMatch(tok, "sizeof ( void )")) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks.. now I guess we should skip the first test.. if (Token::simpleMatch(tok, "sizeof ( void )")) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes you are correct. This is now fixed in my latest commit.

@larseven
Copy link
Copy Markdown
Contributor Author

@danmar : will you be able to merge this PR or do I need to rebase from main first?

@danmar danmar merged commit 9786f1c into cppcheck-opensource:main Mar 25, 2021
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