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

copy paste .clang-tidy from a modern-cpp-template #186

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Arniiiii
Copy link
Contributor

It's a really good default.
It's copy-paste from https://github.com/filipdutescu/modern-cpp-template with one modification: turn off warnings as error for clang-tidy.

It's a really good default.
It's copy-paste from https://github.com/filipdutescu/modern-cpp-template
with one modification: turn off warnings as error for clang-tidy.
Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Thanks for the great config example! I'm not quite sure about its inclusion in the starter yet as I want to avoid too unused configs in derived projects, especially if the checks are not enforced in CI. Unfortunately I don't have much experience with clang-tidy myself, but do you think it would make sense to enforce it as a CI rule?

@Arniiiii
Copy link
Contributor Author

Arniiiii commented Apr 22, 2024

Thanks for the answer and for your efforts to make the starter better!

I think using clang-tidy as warning tool or warning-as-error tool is amazing actually.

I prefer use it as warning-generating tool to find out more problems with code from c++ point of view.

During my use of clang-tidy I found out that with the config the only stuff that makes it warning-as-error is declaring function with an unused parameter and the parameter should be not commented to get warning-as-error behaviour. I genuinely don't know how to disable it, though, I guess it's an OK warning-as-error.

Because of the points above, I guess it would be good to have a github workflow that passes through clang-tidy a codebase to show the error, generate a lot of warnings ( even performance related ) or , if user of the starter changes the .clang-tidy to make a warning into warning-as-error , to generate error.

However, I'm not so proficient with github workflows. I would be pleased If someone can do that or help with it.

@TheLartians TheLartians added the help wanted Extra attention is needed label May 8, 2024
@TheLartians
Copy link
Owner

Thanks for the explanations! Unfortunately I'm currently too busy to help out, but would be happy to include this if we can manage to add a linter step to the workflows!

@Paiusco
Copy link

Paiusco commented May 8, 2024

Seems like you do have clang-format but also don't check it automatically on the gh actions. For clang-tidy seems like here you already have a way to run clang-tidy.

Do you want someone to create a specific step on the actions? There are a few options for that on the marketplace, I can try to test one or two if you prefer.

@TheLartians
Copy link
Owner

clang-format is actually run here, in the style workflow. Currently it's separate from the general tests workflows so it shows up as a unique action on the PR workflows page. Could it make sense to change it into a style and lint workflow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants