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 #12422 (False positive: subtracting pointers in same struct) #5971

Merged
merged 1 commit into from
Feb 11, 2024

Conversation

danmar
Copy link
Owner

@danmar danmar commented Feb 11, 2024

No description provided.

" (void)y[0];\n"
" (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
Owner 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
Owner 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 danmar:main Feb 11, 2024
64 checks passed
else if (vartok->str() == "." || vartok->str() == "::")
else if (vartok->str() == ".")
vartok = vartok->astOperand1();
else if (vartok->str() == "::")
Copy link
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
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