style(clang-tidy): modularize clang-tidy config #1754

bavulapati · 2025-06-23T13:26:44Z

Read blogs and played around with CMAKE_CXX_CLANG_TIDY. Also read source code to understand the clang-tidy config of chromium, llvm and other projects.

We can provide complete context including headers to the clang-tidy tool for static analyzer by running it while building the code. This also makes static analysis first-class citizen of the project, reporting errors before making commits.

For example, misc-internal-linkage check was reporting mostly false positives prior to this. With this, it also considered the headers, where methods/functions were declared constant. Refer #1752

And we should also incrementally add the checks.

These are reported with sourcemeta#1754 Refs: sourcemeta#1654 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Reported with sourcemeta#1754 Refs sourcemeta#1654 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Reported with sourcemeta#1754 Refs sourcemeta#1657 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Reported with sourcemeta#1754 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

These are reported with #1754 Refs: #1654 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati · 2025-06-24T15:33:13Z

@jviotti Need your thoughts on this?
Will resolve the conflicts once we agree on the approach.

jviotti · 2025-06-25T19:57:58Z

I'm not sure I understand the PR. You have deleted the targets that even call ClangTidy, so what is invoking it now? The CI builds also seem to be failing without being able to execute anything. Also a few comments:

Can we still be explicit about the configuration file path, even if the compiler is executing ClangTidy now?
Would always running ClangTidy slow down i.e. development builds? If so we probably want a way to turn it off to only run it on CI

jviotti · 2025-06-25T19:58:27Z

CMakeLists.txt

+endif()
+
+# Add clang-tidy to the targets. Clang-tidy uses the .clang-tidy file for configuration.
+set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_BIN}" "--allow-no-checks")


Can you instead set this variable inside the clang tidy target?

Thought of using global scope

You can still affect the global scope from within a function (there are options for that). That way, we keep everything ClangTidy related in a single file

I considered PARENT_SCOPE but that didn't sound global.
But I can try to encapsulate it.

jviotti · 2025-06-25T19:58:52Z

CMakeLists.txt

+    src/*.h src/*.cc
+    benchmark/*.h benchmark/*.cc
+    test/*.h test/*.cc)
+  sourcemeta_find_clang_tidy() # Locates CLANG_TIDY program


How does it know what files to run it against? i.e. can we ignore dependencies like vendor/uriparser?

It'll use the source files of the target

We are using .clang-tidy file at the root level to disable the checks outside the src/

I wonder how would that work with nested projects. For example, other projects like Hydra will consume Core, and still have their own external libraries that they probably should not lint (like BearSSL). If we override the ClangTidy config in the child, we still have to retain the blacklist from this repo? I'm just concerned about how re-usability and extensibility of the config file will play when other projects consume this. That's why I had a single config file for everything

Considering hydra folder structure, core goes vendored along with hydra's external library dependencies.
With this PR's approach, we disable all the clang-tidy checks at root directory and enable the required checks at the src/folder level.

hydra
|_.clang-tidy(-)
|_src/.clang-tidy(performance-)
|_test
|_vendor--|core/.clang-tidy
|... |bearssl
|..
So, having this hierarchy doesn't mess with each other's config.
All the configuration we do for core doesn't have any role in the configuration of hydra.
If we want to have a single configuration, we can consider symlinks.(Re-usability)
This approach gives extensible too by allowing us to control the configuration at the folder level.
Let me know if you see any issues with this approach.

I can't tell right now. But let's make it work, and then I can attempt i.e. an upgrade on Hydra pointing at the PR and see how it goes

jviotti · 2025-06-25T19:59:20Z

cmake/common/targets/clang-tidy.cmake

-function(sourcemeta_target_clang_tidy)
-  cmake_parse_arguments(SOURCEMETA_TARGET_CLANG_TIDY "REQUIRED" "" "SOURCES" ${ARGN})
-
+function(sourcemeta_find_clang_tidy)


I guess this not only finds ClangTidy but also enables it? Also if we are not making it a target anymore, we should move this file outside of cmake/common/targets

We are not making it a target anymore. We can move it out of targets

Makes sense! Probably under cmake/common

bavulapati · 2025-06-25T20:10:57Z

I'm not sure I understand the PR. You have deleted the targets that even call ClangTidy, so what is invoking it now? The CI builds also seem to be failing without being able to execute anything. Also a few comments:

Can we still be explicit about the configuration file path, even if the compiler is executing ClangTidy now?

Would always running ClangTidy slow down i.e. development builds? If so we probably want a way to turn it off to only run it on CI

This works at the target level. Clang-tidy checks can be enabled/disabled at the target level.

We can be explicit about config path. But it picks up the .clang-tidy from the current directory and combines the config from the parent directory. So, specifying just the current dir config file might be misleading.

Clang-tidy takes time. So, we can have a variable to disable the clang-tidy check.

bavulapati · 2025-06-26T04:58:38Z

execute anything. Also a few comments:

CI builds are failing because of older version of clang-tidy available to them.
We need to install clang-tidy similar to clang-format.

Refs sourcemeta#1754 This makes it use consistent version of clang-tidy on all the platforms. And also uses fairy latest version of clang-tidy, instead of using old versions shipped along with apple llvm and similar distros. Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Refs sourcemeta#1754 Inspired by sourcemeta#1739 This makes it use consistent version of clang-tidy on all the platforms. And also uses fairy latest version of clang-tidy, instead of using old versions shipped along with apple llvm and similar distros. Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati · 2025-06-26T10:57:24Z

I'm not sure I understand the PR. You have deleted the targets that even call ClangTidy, so what is invoking it now? The CI builds also seem to be failing without being able to execute anything. Also a few comments:

Can we still be explicit about the configuration file path, even if the compiler is executing ClangTidy now?

Would always running ClangTidy slow down i.e. development builds? If so we probably want a way to turn it off to only run it on CI

Regarding the slow down, I would propose us to try enabling it by default.
Let's experience the difference and then send a pr to make it optional from our experience.
The builds/targets won't run once they are done and thus it shouldn't make a much of a difference. WDYT?

Refs #1754 This makes it use consistent version of clang-tidy on all the platforms. And also uses fairy latest version of clang-tidy, instead of using old versions shipped along with apple llvm and similar distros. Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

jviotti · 2025-06-26T14:01:00Z

Sounds good. Let's first make it work, and then we evaluate and polish?

) Reported with #1754 Refs #1657 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Reported with #1754 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Read blogs and played around with [CMAKE_CXX_CLANG_TIDY](https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_CLANG_TIDY.html). Also read source code to understand the clang-tidy config of chromium, llvm and other projects. We can provide complete context including headers to the clang-tidy tool for static analyzer by running it while building the code. This also makes static analysis first-class citizen of the project, reporting errors before making commits. For example, `misc-internal-linkage` check was reporting mostly false positives prior to this. With this, it also considered the headers, where methods/functions were declared constant. Refer sourcemeta#1752 And we should also incrementally add the checks. Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati mentioned this pull request Jun 23, 2025

style(clang-tidy): disable performance-enum-size #1751

Merged

bavulapati added a commit to bavulapati/core that referenced this pull request Jun 24, 2025

style(clang-tidy): fix bugprone-branch-clone errors

326be8e

These are reported with sourcemeta#1754 Refs: sourcemeta#1654 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati added a commit to bavulapati/core that referenced this pull request Jun 24, 2025

style(clang-tidy): fix bugprone-branch-clone errors

a482be0

Reported with sourcemeta#1754 Refs sourcemeta#1654 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati mentioned this pull request Jun 24, 2025

style(clang-tidy): fix bugprone-branch-clone errors #1758

Merged

bavulapati added a commit to bavulapati/core that referenced this pull request Jun 24, 2025

style(clang-tidy): fix bugprone-return-const-ref-from-parameter

3a6f62d

Reported with sourcemeta#1754 Refs sourcemeta#1657 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati mentioned this pull request Jun 24, 2025

style(clang-tidy): fix bugprone-return-const-ref-from-parameter #1759

Merged

bavulapati added a commit to bavulapati/core that referenced this pull request Jun 24, 2025

style(clang-tidy): fix bugprone-optional-value-conversion

1c3f43e

Reported with sourcemeta#1754 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati mentioned this pull request Jun 24, 2025

style(clang-tidy): fix bugprone-optional-value-conversion #1760

Merged

jviotti pushed a commit that referenced this pull request Jun 24, 2025

style(clang-tidy): fix bugprone-branch-clone errors (#1758)

18bf995

These are reported with #1754 Refs: #1654 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

This was referenced Jun 24, 2025

style(clang-tidy): fix bugprone-unchecked-optional-access #1762

Merged

Run ClangTidy on GitHub Actions #1640

Open

jviotti reviewed Jun 25, 2025

View reviewed changes

bavulapati mentioned this pull request Jun 26, 2025

Install clang-tidy binaries using CMake similar to clang-format #1786

Merged

jviotti pushed a commit that referenced this pull request Jun 26, 2025

style(clang-tidy): fix bugprone-return-const-ref-from-parameter (#1759

14c16b0

) Reported with #1754 Refs #1657 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

jviotti pushed a commit that referenced this pull request Jun 26, 2025

style(clang-tidy): fix bugprone-optional-value-conversion (#1760)

7945712

Reported with #1754 Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati force-pushed the modularise-clang-tidy-config branch from 86515ba to a974e5e Compare June 27, 2025 04:50

bavulapati added 3 commits June 27, 2025 10:58

Enable performance-* clang-tidy checks

1bfa644

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

remove redundant variable

852043e

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati added 2 commits June 27, 2025 10:59

fix a lint issue

dc40776

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

add missing clang-tidy.cmake

95ef54c

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati force-pushed the modularise-clang-tidy-config branch from 294aaad to 95ef54c Compare June 27, 2025 05:46

bavulapati added 3 commits June 27, 2025 16:36

add missing set

3f5d0f0

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

enable cppcoreguidelines

32e4aca

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

have a single .clang-tidy file

3c33bf6

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>

bavulapati mentioned this pull request Jul 2, 2025

Clang tidy separate ci job #1801

Open

Uh oh!

style(clang-tidy): modularize clang-tidy config #1754

Are you sure you want to change the base?

style(clang-tidy): modularize clang-tidy config #1754

Uh oh!

Conversation

bavulapati commented Jun 23, 2025

Uh oh!

bavulapati commented Jun 24, 2025

Uh oh!

jviotti commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bavulapati commented Jun 25, 2025

Uh oh!

bavulapati commented Jun 26, 2025

Uh oh!

bavulapati commented Jun 26, 2025

Uh oh!

jviotti commented Jun 26, 2025

Uh oh!

Uh oh!