Skip to content

Fix #12422 (False positive: subtracting pointers in same struct)#5971

Merged
danmar merged 1 commit into
cppcheck-opensource:mainfrom
cppchecksolutions:fix-12422
Feb 11, 2024
Merged

Fix #12422 (False positive: subtracting pointers in same struct)#5971
danmar merged 1 commit into
cppcheck-opensource:mainfrom
cppchecksolutions:fix-12422

Conversation

@danmar
Copy link
Copy Markdown
Collaborator

@danmar danmar commented Feb 11, 2024

No description provided.

" (void)y[1];\n"
" (void)y[2];\n"
"}\n");
ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (error) The address of local variable 'a' is accessed at non-zero index.\n"
Copy link
Copy Markdown
Collaborator Author

@danmar danmar Feb 11, 2024

Choose a reason for hiding this comment

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

It's unfortunate that it does not warn anymore but I feel the implementation and message was sloppy:

  • Variable 'a' is not a local variable. The message should not say that it's "local".
  • I guess it should have warned about 's.a' instead. "address of 'a'" is not correct.
  • It does not matter if it is a local or global variable what is important is that it's a plain int variable.
  • Because the lifetime is irrelevant I am skeptic about using the lifetime values in the checker wouldn't it be better to rely on tokvalue?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

about these comments: I created ticket https://trac.cppcheck.net/ticket/12428 so we can act on those later when we have time.

@danmar danmar merged commit 3efc6d3 into cppcheck-opensource:main Feb 11, 2024
Comment thread lib/valueflow.cpp
else if (vartok->str() == "." || vartok->str() == "::")
else if (vartok->str() == ".")
vartok = vartok->astOperand1();
else if (vartok->str() == "::")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this change makes sense overall, though there maybe some places expecting a variable(ie getLifetimeVariable). So I was always concerned about causing issues, but it looks like only one test failed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah wait nevermind, this is changing a.x to point to a instead of x. I thought it was changing it to point to a.x. This would be a better change, but I guess there is a lot more test failures.

For a quick fix, it seems like #5970 is a better solution for a quick fix.

@danmar danmar deleted the fix-12422 branch April 12, 2024 16:00
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.

2 participants