Enable clang-tidy checks for clang gmake2

[theComputeKid]

Some time ago, I added build-time code checking for MSVC (#2190) and static analysis via clang-tidy (#2187). I also wanted to add this for the gmake2 generator, specifically for the clang toolset.

However, having read through the code, there are multiple different ways to achieve this, and I am not sure what to do. I am happy to add this feature myself if I can get some guidance from people familiar with the codebase:

1. How can I limit enabling clang-tidy to just clang?
2. What changes should be made in the gmake2 generator to make this happen?

To mimic cmake, should I add a second build command here, protected by a conditional:

    function cpp.initialize()
        rule 'cpp'
            fileExtension { ".cc", ".cpp", ".cxx", ".mm" }
            buildoutputs  { "$(OBJDIR)/%{file.objname}.o" }
            buildmessage  '$(notdir $<)'
            buildcommands {'$(CXX) %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"'}

The second build line will contain the clang-tidy invocation:

    function cpp.initialize()
        rule 'cpp'
            fileExtension { ".cc", ".cpp", ".cxx", ".mm" }
            buildoutputs  { "$(OBJDIR)/%{file.objname}.o" }
            buildmessage  '$(notdir $<)'
            buildcommands {
                           '$(CXX) %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"',
                           'clang-tidy -- all necessary clang-tidy arguments along with compiler flags.'}

What would a conditional look like? Or, do I add a prebuild command instead?

Nice to haves:

1. We would also need to inform clang-tidy where to read the .clang-tidy file from as it may not necessarily be in a standard location or inside the output project folder.
2. Specify location of clang-tidy. Or perhaps this can be taken from llvmdir? But that is only supported for msvc.

[tritao]

Hey @theComputeKid, this would be a really nice feature to have, at least I've been wanting to have this in Premake lately too.

• How can I limit enabling clang-tidy to just clang?

At first I thought you should be able to check the toolset and only generate this if its a clang-based one.

But upon second thought, is there any specific reason why we should limit clang-tidy rule generation to Clang-based toolsets? As long as clang-tidy is in your PATH, or you pass the path to a specific binary to Premake, then IMHO you should be able to use it.

Assuming we have a clangtidy command to control it, as you seem to already have added, then the user can set it up as they wish. For example sake:

clangtidy "Off"

filter {"toolset:clang" }
    clangtidy "On"

filter {"toolset:gcc" }
    clangtidy "Off"

Maybe whats needed is a new command to set its path, say clandtidypath, and would default to the PATH.

2. What changes should be made in the gmake2 generator to make this happen?

Personally I think it would be good to split this into a separate rule for checking, that is generated by default but is only run on demand, say make lint or make clang-tidy.

But I can also see it being useful as an always on rule that runs every time code is compiled. Actually I have not used clang-tidy too much yet, so based on your experience, is it something you can always have turned on and not affect build performance too much? Depending on that maybe a separate rule is not necessary?

[theComputeKid]

Hi @tritao! Thanks for the response.

Clang-tidy is built around clang tooling and it works with 90% of GCC's features, but sometimes errors in more advanced features, for example, it expects PCH in clang's binary format rather than GCC's and then it gets confused. In my own CMake builds, I only keep clang-tidy on for clang builds. However, we can also do as you say, people can turn off clang-tidy for gcc, based on whether their GCC build line works with clang-tidy. I'll do that then.

As for running clang-tidy as part of the build, that is tricky. On one hand, current premake MSVC and CMake run it as part of the build, while they are compiling the file itself. You are right that this 2x's the time taken to build, unfortunately, for consistency, it would be better to do what CMake and premake MSVC does.

In which case, should I add it to build commands such as (pseudocode):

function cpp.initialize()
        rule 'cpp'
            fileExtension { ".cc", ".cpp", ".cxx", ".mm" }
            buildoutputs  { "$(OBJDIR)/%{file.objname}.o" }
            buildmessage  '$(notdir $<)'
            buildcommands {'$(CXX) %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"'}
if clangtidy then
            buildcommands { clang-tidy invocation }
end

Or, do I add a post build command, such as:

function cpp.initialize()
        rule 'cpp'
            fileExtension { ".cc", ".cpp", ".cxx", ".mm" }
            buildoutputs  { "$(OBJDIR)/%{file.objname}.o" }
            buildmessage  '$(notdir $<)'
            buildcommands {'$(CXX) %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"'}
if clangtidy then
            postbuildcommands { clang-tidy invocation }
end

MSVC does not have per file clang tidy (I think, I don't remember). It just switches it on for the whole project.

My preference would be to add it to build commands, so clang tidy for a file runs immediately after the build for the file has completed, instead of running after the whole project has been built. Thoughts?

[Jarod42]

rule doesn't have postbuildcommands, buildcommands seems fine though.

We would also need to inform clang-tidy where to read the .clang-tidy file from as it may not necessarily be in a standard location or inside the output project folder.

.clang-tidy is looked recursively from source to parent directory, as clang-format IIRC.

[theComputeKid]

You are right @Jarod42 and that is what I was thinking when I made the PR, so can you look at #2247 and help me come up with an appropriate solution, in case you think my proposal is not good enough? Thanks.