-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Added more failing tests for whole program analysis #6551
base: main
Are you sure you want to change the base?
Conversation
I was getting to those hopefully soon. As you can see the existing PR was approved/merged just today. And I just posted my own follow-up earlier. I am also not feeling too well at the moment so I was only able to post/adjust pre-existing work I did and have not been able to do any new stuff yet. |
Ah, you opened the other PR against a branch of mine which I already deleted. Didn't even realize that. My tests have finally been merged and I pushed the first bugfix. After that has been merged I will dissect your tests and pull in what is still relevant. |
Just checking-in after seeing release note:
After rebasing, tests below still fail due to the ordering of defines in the compile_db. This means plugins and
and
With |
@@ -0,0 +1,18 @@ | |||
// cppcheck-suppress misra-c2012-2.3 | |||
typedef struct { | |||
int x; // cppcheck-suppress unusedStructMember |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"unusedStructMember" is not a whole program analysis checker. so I think putting that in this whole program test is suspicious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think unusedStructMember
, unusedVariable
, etc. should be checked against different configurations, similar to unusedFunction
.
However this was not main point here. - For lack of better example, I used structure typedef to exercise one of the misra's system level rules that I was familiar with i.e. Rule 2.3 - a project should not contain unused type declarations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should ideally check these against different configurations in the "unused*" checkers but not sure if I think this is the proper test for that. I prefer short tests that are more to the point. the test for unusedStructMember wouldn't require a unused function for instance and it's not required with a compile commands. Spontanously I would prefer testing it in testrunner.. however I don't rule out completely that maybe it makes sense to write a pytest that executes the whole cppcheck tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine that most "style" checkers would ideally look at all the configurations. A "known condition result" checker can also generate false positives for instance.
test/cli/whole-program_test.py
Outdated
f = os.path.basename(fp) | ||
|
||
for defines in (definesList[i] if len(definesList) > i else [None]): | ||
obj = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt that in a real world project a compile commands will contain multiple entries for the same file compiled with different flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That actually exists in our own project. We compile the same source files within different scopes. There also can be multiple commands within a single entry I think. We have a workaround for that and it might be an extension outside of the actual "documentation" (there is no "real" specification). Need to look that up again. We might also need to document and make things opt-in.
This also plays into the de-duplication of stuff which is still not complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There also can be multiple commands within a single entry I think
Arguments can be provided in an array instead of a string maybe that's what you're thinking about?
So if it is allowed with multiple entries for the same file in a compile commands then it's a good thing to test this explicitly. But I am still not sure if I like this test if that is explicitly what you want to test. I wonder if we can write more specific tests in testrunner that checks that the file is loaded correctly etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot remember - I will look it up later and file a ticket.
This is still in my scope but I got side tracked with some more fundamental stuff again. I also didn't feel like looking at creating new Python tests in the past few weeks... |
This PR adds a failing test against this issue: https://trac.cppcheck.net/ticket/12971
@pytest.mark.xfail(strict=True)