feat: use clang-tidy lint readability-identifier-naming #13642
Conversation
|
This is still all green, but there are 10 warnings. Not sure if clang tidy gives up at some point? Fixing all occurrences and the required review and merge conflicts is a waste of time IMHO. We can better make use of the time implementing a new features. |
Why only 10? I do see loads of warnings once I unfold the build log (including ones warning about faulty configs, my mistake).
Not sure, I can look into it but I don't think there is (since clang-tidy is a built tool and likely has no concept of git). Github itself however does put the warnings into the "files changed" tab directly at the right location during review. So at least it should be noticeable during review without being too distracting. |
Swiftb0y
force-pushed
the
feat/clang-tidy-naming
branch
from
767a7e7 to
4abc2b2
Compare
|
I added a script that runs clang-tidy on the diff pre-commit. that also allows us to remove the clang-tidy CI which is a waste of energy anyways. |
|
So this technically works now. I'd like to get some feedback on this first before I bother with doing the CI env properly. |
Ah. It seems to be the limit of the GitHub parser. The issue is that we will be dragged away from important warnings if we see only 10 of these renaming warnings. So we may wrap clang-tidy and filter the untouched lines. Something like "clang-tidy-diff.py" that calls git and clang-tidy.y and maybe uses also the dependcy info of the make/ninja. A small fun project ?? |
|
Oh I see you have a different solution. This is interesting. Do we still have the option to check all lines from time to time without readability-identifier-naming. Or is it no longer required? |
Well, you can just remove the check from the config and run clang-tidy manually. Now that clang-tidy only complains about the diff, I'm planning on enabling a lot more checks since those are generally useful and have just previously been unfeasible for our large codebase at once. |
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
| @@ -36,6 +33,34 @@ jobs: | |||
| run: | | |||
| git config --global --add safe.directory "${GITHUB_WORKSPACE}" | |||
| git config --global --list | |||
| - name: Install build dependencies | |||
| run: tools/debian_buildenv.sh setup | |||
There was a problem hiding this comment.
This is a quite costly operation IMO - how do you feel about updating the Docker image instead? Note that would could also make the container in #14018 pushed in GHCR and reuse it in our CI? It may actually be quite good as we could use it build (Ubuntu) as well and a save a significant amount of bandwidth/resource consumption
There was a problem hiding this comment.
yeah, I agree. this was just a quick and dirty PoC that has stalled now (because of issues like these). Updating the docker would def. be preferred.
There was a problem hiding this comment.
@mixxxdj/developers any objection in me extending the scope of #14018 and publish devcontainer in Mixxx GHCR repo?
github-actions
bot
removed
the
stale
This is a best effort attempt to encode our naming rules in clang-tidy. This obviously creates a lot of warnings and we need to decide on how to resolve them. should I just go through in fix them all, risking merge conflicts?