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

[ESD-2180] Improved clang-tidy module #101

Merged
merged 9 commits into from
Sep 30, 2021

Conversation

woodfell
Copy link
Contributor

@woodfell woodfell commented Sep 24, 2021

There is a bit going on in here.

First there is a new module ListTargets.cmake which can enumerate all targets in a source tree based on certain criteria

Second there is a new property defined on targets SWIFT_TYPE which is filled in by all the swift_add_* functions. This property is one of the criteria which ListTargets can filter on.

There is a new function swift_validate_targets which combines the above two items to make sure that all compilable targets defined in a project were added with swift_add_* rather than the native cmake add_*. In the future this function can be expanded to provide extra validation of targets.

Finally there is a completely rewritten ClangTidy module. Rather than having to pass a git GLOB pattern to swift_setup_clang_tidy a replacement function swift_create_clang_tidy_targets uses ListTargets and the new property to be far more smart about building up the list of source files to pass to clang-tidy

All targets defined within a repository have their own clang tidy targets, eg clang-tidy-starling, clang-tidy-pvt-engine. Fixes are exported to fixes-<target>.yaml

clang-tidy-all still exists and lints all "core" parts of starling, anything defined with swift_add_executable or swift_add_library

clang-tidy-world lints absolutely everything defined in a repository, all core targets, tools, tests, test libraries and so on.

Each clang-tidy-* target has a corrosponding clang-tidy-<target>-check target which returns an error code should any errors be generated.

The old "diff" clang-tidy targets no longer exist.

All clang-tidy targets are run in parallel using a script from the LLVM project.

.clang-tidy is now autogenerated during swift_setup_clang_tidy, as this module is updated in other repositories all versions of .clang-tidy can be removed and we can finally have proper standardisation across key projects. In recognition that some other projects really do want their own configs this behaviour is controllable and can be disabled if required.

@@ -0,0 +1,344 @@
#!/usr/bin/env python
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file taken almost verbatim from LLVM.
The only change is to not write an empty fixes file if no errors were generated

Copy link
Contributor

@RReichert RReichert left a comment

Choose a reason for hiding this comment

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

Really awesome work. Minor nits and comments, but looks good to me.

ClangTidy.cmake Outdated
@@ -1,308 +1,216 @@
#
# Copyright (C) 2021 Swift Navigation Inc.
# Contact: Swift Navigation <[email protected]>
# Copyright (C) 2021 Swift Navigation Inc. Contact: Swift Navigation <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

were these intentional changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I made the mistake of running cmake-format before committing everything. It cocked up the entire file. I'll revert this header block but won't bother with the rest of the file even though it looks like garbage

#
# will explicitly disable these targets from the command line at configure time
# swift_create_clang_tidy_targets will generate a .clang-tidy file in the project source directory which contains the Swift master config for clang-tidy. There is no need for
# repositories to maintain their own version of .clang-tidy, it should be added to .gitignore in each repository to prevent being checked in. Pass the option
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, like the idea. I'd probably ask on slack if anyone relies on the .clang-tidy on their repo. I personally dont, but I know that CLion does have some nice integration with clang-tidy and reports these errors in realtime in the IDE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I use something similar. It's pretty important to keep .clang-tidy in the repo root thanks to the braindead way clang-tidy looks for its config. It would be so much easier if you could pass the path to a config file as a command line argument, but for whatever reason they didn't implement it like that.

Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

LGTM.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@woodfell
Copy link
Contributor Author

I've had to add the original ClangTidy.cmake back in to this PR (as OldClangTidy.cmake) due to a problem in some higher level repositories that haven't been converted over to use the swift_add_* functions yet. It was going to be too much effort to convert all of them as well (and probably cause a fair amount of annoyance due to stricter compile options) so they can keep the old module for the moment.

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