In general this seems like it would be useful.

Some open topics.

• This patch currently warns but doesn't fix the code (it can automatically add braces for eg), is the intention to enable this at some point? Or support it as a way of automating the edits (but keep it off by default).
• If it's important to get gcc flags working with clang-tidy, we could point clang-tidy to a wrapper script that filters the flags.
• It'd be interesting to see if this could be a separate target from building, so it can be run without having to always/disable.

@Sybren A. Stüvel (sybren) ,

In my understanding const float* will not trigger readability-avoid-const-params-in-decls. It will be triggerred on int foo(const int bar);. Having int foo(const int bar) {} in the definition is something we do want to support (so bar is not modified by accident in the middle of the function).

If we can enable readability-avoid-const-params-in-decls in a way that in definition we can still have const int foo i have no issue enabling the warning.

The rest I also agree they are handy and useful and something what feels like we should have. Is just from own experience they are a bit noisy, so would need team effort to tackle them ;)

I also feel specifics of what warnings to enable or disable we should have in case-by-case basis, maybe in blender-coders. Feel like that'd be more efficient, especially if we have fundamentals landed.

@Campbell Barton (campbellbarton) ,

This patch currently warns but doesn't fix the code (it can automatically add braces for eg), is the intention to enable this at some point? Or support it as a way of automating the edits (but keep it off by default).

I am aware there is an automated mode for clang-tidy, but I never used it. For something simple like braces it could be handy, for the rest I feel like it is to be double-checked by a developer.

If it's important to get gcc flags working with clang-tidy, we could point clang-tidy to a wrapper script that filters the flags.

There might be a way to avoid the wrapper script, or could be a way to affects flags passed to clang-tidy using CMake. Not sure.
But indeed wrapper script can be a fallback solution.

It'd be interesting to see if this could be a separate target from building, so it can be run without having to always/disable.

I personally find is easier to have separate build folder. However, if there is a non-nasty way to enable it as a target I've got nothing against it.

On Windows, as far as I remember, there is clang-tidy distributed together with LLVM package. It might work to pass the executable to CMake via CLANG_TIDY_EXECUTABLE CMake argument, but I did not test this.

Clang and MSVC take rather different command line parameters, i gave it a quick whirl and clang-tidy did not seem to have a good time being fed msvc switches, the clang based build may work with it but that broke recently with some of the TBB changes jeroen did, which i still need to fix one day, but i'll be honest, the clang based build emits about 150k warns whatever clang-tidy will add to that will just get lost in the noise. MS ships with some clang tidy support in the IDE but that also threw a fit on some of the options we are using.

so yeah clang-tidy on windows is currently "not great" I'd just hide the WITH_CLANG_TIDY behind a if(NOT WIN32) for now and focus on getting a ruleset that we like.

We could ship clang-tidy with the precompiled libraries like we do clang-format. Maybe the next time we upgrade LLVM/Clang, it'll be easier then since llvm switched to using a single git repo for llvm, clang, clang-tools-extra.

Maybe a bit wrong place for this discussion, but let me at least briefly dump my thoughts here, and then we can move discussion somewhere else.

I think we should have such developers tool in SVN (including clang-tidy, clang-format) so then the entire team is synchronized at versions. Is not so important for clang-tidy on Linux (is easy to install from repo), but clang-format can easily cause fight-over-style as there some minor behavior differences in corner cases.

The only thing I wouldn't mind if that is somehow decoupled from LLVM version we use for OSL. From the past experience OSL was quite conservative in LLVM requirement (in a way that OSL wanted old LLVM) and I don't feel like being blocked for clang-format and clang-tidyupdate because of OSL.

OSL changed this stance and generally supports building with the latest llvm. On windows we currently use clang-format from the svn libs, clang-tidy is somehow not build in our deps builder, will have to take a peek why that is.

In D7937#192158, @Sergey Sharybin (sergey) wrote:

Having int foo(const int bar) {} in the definition is something we do want to support (so bar is not modified by accident in the middle of the function).

+1

If we can enable readability-avoid-const-params-in-decls in a way that in definition we can still have const int foo i have no issue enabling the warning.

That would mean:

// Declaration that'll trigger the warning:
void const_scalar(const float arg);

// Declaration that is fine:
void const_scalar(float arg);

// Definition that can be used with either declaration without warning:
void const_scalar(const float arg) { … }

For the config file, you can avoid all the trailing backslashes by using a > sign in the appropriate place and then indenting the string:

Checks: >
    -*,
    readability-*,
    -readability-uppercase-literal-suffix,
    -readability-magic-numbers,
    -readability-isolate-declaration,
    -readability-convert-member-functions-to-static,
    -readability-implicit-bool-conversion,
    -readability-avoid-const-params-in-decls,
    -readability-simplify-boolean-expr,
    -readability-make-member-function-const,

    -readability-misleading-indentation,

    -readability-else-after-return,
    -readability-braces-around-statements,
    …

In D7937#192191, @Ray Molenkamp (LazyDodo) wrote:

OSL changed this stance and generally supports building with the latest llvm. On windows we currently use clang-format from the svn libs, clang-tidy is somehow not build in our deps builder, will have to take a peek why that is.

clang-tidy is part of clang-extra-tools which is in its own repo separate from clang. At least it was until recently. In the most recent version llvm, clang and clang-extra-tools are all in the same repo.

In D7937#192187, @Sergey Sharybin (sergey) wrote:

I think we should have such developers tool in SVN (including clang-tidy, clang-format) so then the entire team is synchronized at versions. Is not so important for clang-tidy on Linux (is easy to install from repo), but clang-format can easily cause fight-over-style as there some minor behavior differences in corner cases.

Agreed.

The only thing I wouldn't mind if that is somehow decoupled from LLVM version we use for OSL. From the past experience OSL was quite conservative in LLVM requirement (in a way that OSL wanted old LLVM) and I don't feel like being blocked for clang-format and clang-tidyupdate because of OSL.

I think this is something we can do if the need arises. As @Ray Molenkamp (LazyDodo) mentioned this has not been a problem for recent OSL versions.

@Sybren A. Stüvel (sybren) That's good to know! Poked you in the chat to figure out some really stupid aspects of the change though.

@Brecht Van Lommel (brecht), I wasn't aware OSL has no issues with recent LLVM. If that't the case then indeed no need to make dependencies compilation more complicated than it should be.
Can you mention which of the feedback you are still awaiting to be addressed?

Am all for this kind of dev help as well.

Trying the patch here, I get a linking error, but this seems related to clang itself (using 9.0.1 here), not the tidy tool.

/usr/bin/ld: -f may not be used without -shared
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I've never built Blender with Clang before. Building current master fails with the same error as @Bastien Montagne (mont29) is getting. However, when I disable USD, I get lots of undefined reference errors from the linker (I tried with the CentOS libs from SVN and my own GCC-compiled libs, maybe I should rebuild the dependencies with Clang too?):

...
/usr/bin/ld: lib/libextern_ceres.a(schur_eliminator_2_d_d.cc.o): in function `.omp_outlined.':
schur_eliminator_2_d_d.cc:(.text+0x26): undefined reference to `__kmpc_global_thread_num'
/usr/bin/ld: schur_eliminator_2_d_d.cc:(.text+0x87): undefined reference to `__kmpc_dispatch_init_4'
/usr/bin/ld: schur_eliminator_2_d_d.cc:(.text+0xa5): undefined reference to `__kmpc_dispatch_next_4'
/usr/bin/ld: schur_eliminator_2_d_d.cc:(.text+0xe2): undefined reference to `__kmpc_dispatch_next_4'
/usr/bin/ld: lib/libextern_ceres.a(schur_eliminator_2_d_d.cc.o): in function `.omp_outlined..6':
schur_eliminator_2_d_d.cc:(.text+0x2fc): undefined reference to `__kmpc_global_thread_num'
...

Please stick to the subject of changes in the code reviews. It is not helpful to go into discussions of subject which is not caused or affected by the specific change.