Add clangtidy for gmake2 #2247
Add clangtidy for gmake2 #2247
Conversation
| -- code checking must be before the build, so that if the code checking | ||
| -- fails, the build is not completed, and hence, a re-run will trigger | ||
| -- the code check again. | ||
| '$(CLANG_TIDY) "$<" -- %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"', |
There was a problem hiding this comment.
How difficult would it be to make it so this isn't emitted when clang-tidy isn't requested, rather than always emitting and just "ignoring" it sometimes?
There was a problem hiding this comment.
@nickclark2016 : I knew someone would ask this question, so I already pre-empted the answer in my original PR description: Unfortunately, at the point where the rules are decided, we do not have access to the value of the clangtidy property. You can see it here in this function, there is no information to play with. And then we have other issues as well like turning clang tidy on/off depending on the project configuration.
Then again, I am an outsider to the premake team, you probably know more about the code and can suggest better.
There was a problem hiding this comment.
From "rule" point of view, we would need
- 2 rules:
rule 'cpp'andrule 'cpp_with_clang_tidy' - remove the
fileExtensionsand do the dispatch "manually".
There was a problem hiding this comment.
Could you please give some psuedo code to explain what you are saying? I'm not sure I understand.
There was a problem hiding this comment.
I mix with customcommand as fileextension cannot be removed...
Idea is to customize rule to handle clangtidy:
if you read https://premake.github.io/docs/Custom-Rules/
we might add propertydefinition to handle clangtidy, then we have to activate that property for each filecfg with clangtidy.
Not sure at which point it is doable without hack.
I will try to add clang-tidy support to premake-ninja, but it doesn't use (premake) rule internally but has "intrinsic" rule.
Above (adapted) method should do the trick for me.
There was a problem hiding this comment.
I see. This sounds very messy. Not sure if there is an acceptable solution.
There was a problem hiding this comment.
Sounds like there is no elegant fix for adding clang tidy for gmake2. It was fun trying to get it to work but no worries, feel free to close this PR.
There was a problem hiding this comment.
I succeeded with ninja, and failed with gmake currently (to have a correct per-file per-config)
Some features are already partially working (per-file not taken into account for some generator).
Might be better to have partial working solution than no solution at all...
BTW, I will see if I can improve your PR with my suggestion.
There was a problem hiding this comment.
Awesome, thanks!
theComputeKid
force-pushed
the
clangtidy
branch
from
db1ae7f
to
2d90b06
Compare
| -- code checking must be before the build, so that if the code checking | ||
| -- fails, the build is not completed, and hence, a re-run will trigger | ||
| -- the code check again. | ||
| '$(CLANG_TIDY) "$<" -- %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"', |
There was a problem hiding this comment.
-o "$@" -MF "$(@:%.o=%.d)" -c "$<" seems superfluous/wrong.
| @@ -425,6 +432,13 @@ | |||
| p.outln('ALL_CXXFLAGS += $(CXXFLAGS) $(ALL_CPPFLAGS)' .. flags) | |||
| end | |||
|
|
|||
| function cpp.clangtidy(cfg, toolset) | |||
| if cfg.clangtidy ~= nil then | |||
There was a problem hiding this comment.
"Off" (or false if there is some translation phases for that parameter) would wrongly enter in that case.
| ]] | ||
| end | ||
|
|
||
| function suite.clangtidyOff() |
There was a problem hiding this comment.
| function suite.clangtidyOff() | |
| function suite.clangtidyDefaulted() |
currently, and you might add a dedicated test for "Off", especially when code suggest that it is wrongly handled ;-)
ClangTidy code checks have been added to the Visual Studio generator in #2187. A request for the equivalent in gmake2 has been made in #2245 as well as in this comment. Static analysis is a necessary tool for software developers.
This PR enables clang tidy for gmake2 at the configuration level. It adds a build command to the cpp rule enforcing the checks.
When clangtidy is Off, the build command enforcing clang tidy expands to a no-op, and it is skipped. When clangtidy is On, the command first runs clang tidy on the file, and then compiles it if the checks are successful. This method of working allows finishing the build early if clang tidy fails. This is also how the Visual Studio generator works, as well as how CMake implements clang tidy.
Closes #2245
I may need more help with this PR. Due to the invasive nature of the changes (a new build command is added to the makefile, which may understandably make the code reviewers uncomfortable), the following alternatives were considered:
clangtidyis On. I don't know how to do this, becausecfg.clangtidydoes not seem to be available duringcpp.initialize.Because of these reasons, I instead decided to go with the implementation in this PR.
Future work is possible to make clang tidy in premake more powerful:
Did you check all the boxes?
closes #XXXXin comment to auto-close issue when PR is merged)