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

Clang tidy showcase #4352

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

Conversation

teksturi
Copy link
Contributor

@teksturi teksturi commented Dec 16, 2023

This PR is just showcase what kind of things clang-tidy could do. So I am not suggesting we take all of these checks in use. Currently not buildable as it makes mistakes with some cases. It did also found many bugs. I have to send fixes to those first before doing real clang-tidy things.

We just need to get these thing automated so they do not happened again. Note that these are only checks which clang-tidy can fix automatically. There is also bunch of good checks which can give just warning.

This was done for #4348

@matejk
Copy link
Contributor

matejk commented Dec 17, 2023

@teksturi, this is excellent!

How would clang-tidy be automated?

Shall it be added as the last build target in CMake and make the build fail?

Or is it safe for automatic fixes to be applied also?

@obiltschnig
Copy link
Member

We should enable clang-tidy checks as part of normal CI.
In the first step, just print the warnings but not fail. After all things are fixed, make the build step fail if clang-tidy reports issues. Probably need to clarify what warnings or errors would cause the build to fail also (maybe some are acceptable/unavoidable).
I don't want automatic fixes for now.

@teksturi teksturi mentioned this pull request Dec 17, 2023
5 tasks
@matejk matejk deleted the branch pocoproject:main April 15, 2024 11:20
@matejk matejk closed this Apr 15, 2024
@matejk matejk reopened this Apr 22, 2024
@matejk matejk changed the base branch from devel to main April 22, 2024 06:56
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