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

check: Add supression file for false positives and pending fixes for cppcheck tool #4766

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

ymdatta
Copy link
Contributor

@ymdatta ymdatta commented Nov 27, 2024

For past few months, I have been working on sorting errors thrown by cppcheck static analysis tool into true positives and false positives manually and fixing the true positives. I believe I was able to fix all the issues arising from the tool.

The file added as part of this PR contains information on false positives and some true positives (for which pull requests are opened and are in discussion. #4702, #4638, #4500, #4499). For false positives which are not obvious, I have added explanations on why there tagged as such.

Once this file is added to the project, we can work on a CI action which takes this suppression file and runs cppcheck on any new changes to the project, thus finding new issues if there are any.

Sample way to run using the suppressions file is: cppcheck --suppressions-list=.cppcheck-suppressions <path>

Documented each suppression issue with comments to distinguish between
false positives and true positives awaiting resolution.

For the false positives suppressions, appropriate information is
provided on why those were considered as false positive.

True positives will be removed from the suppression file once
their corresponding fixes(OSGeo#4702, OSGeo#4638, OSGeo#4500, OSGeo#4499) are merged.

Run:

`cppcheck --suppressions-list=.cppcheck-suppressions <path>`

Signed-off-by: Mohan Yelugoti <[email protected]>
@ymdatta ymdatta force-pushed the cppcheck_supressions_file branch from 7338d07 to e2088ae Compare November 27, 2024 02:17
@ymdatta
Copy link
Contributor Author

ymdatta commented Nov 27, 2024

@echoix : Would love to get your thoughts on this.

@ymdatta ymdatta changed the title cppcheck: Add supression file for false positives and pending fixes check: cppcheck: Add supression file for false positives and pending fixes Nov 27, 2024
@ymdatta ymdatta changed the title check: cppcheck: Add supression file for false positives and pending fixes check: Add supression file for false positives and pending fixes for cppcheck tool Nov 27, 2024
@echoix
Copy link
Member

echoix commented Nov 27, 2024

Does cppcheck run without any other config needed other than that call? Does it need to compile or it can work just like that?

For the file itself, I just skimmed through, and since there's no CI or nothing that has a negative impact, I'm willing to merge as is, especially if you have accumulated that file through the weeks. If you're not sure that implementing a CI is stable enough yet, don't worry, we could have multiple levels to introduce it. We could:

  • not have it a required check, so failing doesn't matter while we are stabilizing it.
  • Have it run only when a certain label is added (manually), so you could experiment it.
  • Add a workflow_dispatch trigger type, so it can be manually launched. It isn't as easy to launch for a PR, but might be helpful when experimenting if you'd want it merged before it is ready.
  • Lastly, have a never ending PR and only merge it when perfect, if perfection is possible (note my sarcasm here :p )

I imagined having these kind of tools, like clang analysers in the same workflow, but possibly in other jobs, as the gcc standards check one.

Tell me how confident you are with this file, and we'll merge accordingly (it could be as soon as tomorrow)

@echoix echoix merged commit a9a4cec into OSGeo:main Nov 29, 2024
25 of 31 checks passed
@github-actions github-actions bot added this to the 8.5.0 milestone Nov 29, 2024
@ymdatta
Copy link
Contributor Author

ymdatta commented Nov 30, 2024

@echoix : Sorry, I missed the comment during the holidays.

I am fairly confident about the file. This doesn't need any other configuration apart from just installing the tool and running it.

I would say it's a good idea to have it as a check for PRs, but not as a required check to see if it hits anything I might have missed. This way, I can get that stabilized.

@ymdatta ymdatta deleted the cppcheck_supressions_file branch November 30, 2024 22:35
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