feat: add to check headers

[dayfoo]

trafficstars

Header files, which are not exposed, are not checked by clang-tidy via bazel_clang_tidy.

e.g.

cc_library(
    name="test",
    srcs=["*.cpp"], # <- check
    hdrs=["*.hpp"], # <- not check
)

It seems that other similar projects checks header files.

https://github.com/grailbio/bazel-compilation-database/blob/6b9329e37295eab431f82af5fe24219865403e0f/aspects.bzl#L78

https://github.com/hedronvision/bazel-compile-commands-extractor/blob/1d21dc390e20ecb24d73e9dbb439e971e0d30337/refresh.template.py#L109

[erenon]

Hi, thanks for the pr. Can you tell me more about your use-case? Normally, headers files should be covered by clang tidy, simply by running it on source files that include those headers.

[dayfoo]

In my case, any header files, which are not specified in srcs but specified in hdrs, are not checked. If we'd like clang-tidy to check header files, I guess we need to specify -header-filter. But, it seems that -header-filter isn't specified anywhere. For example, in my understanding, if we'd like to specify header files as internal usage, I think we usually specify those header files in hdrs. And, we'd like to check even internal header files with clang-tidy as well.

[erenon]

For example, in my understanding, if we'd like to specify header files as internal usage, I think we usually specify those header files in hdrs

This is incorrect. hrds is for public headers: https://docs.bazel.build/versions/main/be/c-cpp.html#cc_library.hdrs

We discussed this internally, and found the following counter arguments:

• Some headers cannot be compiled on their own
• It is not obvious, if a given header should be compiled as c or c++
• Headers are covered transitively by linting source files that include them. Header filter can be set in the .clang-tidy configuration file. linting headers in addition to sources that include them is a waste of resources.

[dayfoo]

Oops. sorry for my misunderstanding.

Regardless how to specify header-filter, I think the paths to search header files are specified with -I or -quote to clang-tidy. If so, the following case, what happen?

cc_library {
  srcs = ["main.cpp"],
  hdrs = ["include/test.hpp],
  includes = "include",
}

include is specified as -isystem , and even if we specify header-filter , I think clang-tidy deal with include/test.hpp as system header and it ignores include/test.hpp.

[dayfoo]

It is not obvious, if a given header should be compiled as c or c++

In my humble opinion, for example, we may be able to deal with h file as c.

[dayfoo]

Some headers cannot be compiled on their own

In this case, I guess you assume external library headers. If so, those files should be defined as cc_library and then it should be specified in deps in cc_library or cc_binary etc. And then, I think finally it is specified with -isystem to clang-tidy in bazel_clang_tidy.

Of course, in this case, it may restrict the users how to write BUILD file...

[chandlerc]

Sorry to bring this thread back to life, but we're looking at incorporating this or something like this because without it we're missing coverage.

For example, in my understanding, if we'd like to specify header files as internal usage, I think we usually specify those header files in hdrs

This is incorrect. hrds is for public headers: docs.bazel.build/versions/main/be/c-cpp.html#cc_library.hdrs

Yep, but note that header-only libraries can easily end up needing this to get coverage.

We discussed this internally, and found the following counter arguments:

• Some headers cannot be compiled on their own

Those shouldn't be in hdrs in Bazel, they should be in textual_hdrs: https://bazel.build/reference/be/c-cpp#cc_library.textual_hdrs

The expectation of hdrs is specifically that they can be parsed in a stand-alone mode exactly as done by tools like clang-tidy.

• It is not obvious, if a given header should be compiled as c or c++

This is true, but both Bazel and other projects make a pretty clear default of headers being at least allowed to be #include-ed into C++ code. Even for C headers, it is quite unusual for them to be incorrect when included into a C++ context. And these are explicitly public headers, and so it seems a very reasonable default to expect them to be parsable as C++ and checked with clang-tidy as C++ (and in fact, I think clang-tidy itself has a mode that picks a good default when given a header).

And for the rare case, there are flags that can be put into copts to force using C. Or the header can be placed into textual_hdrs to turn off any assumptions about how the header will be used.

• Headers are covered transitively by linting source files that include them. Header filter can be set in the .clang-tidy configuration file. linting headers in addition to sources that include them is a waste of resources.

See above, and you can also see the linked PR for an example, from our direct use of this tool we ended up needing this to get all our files covered.

WDYT, open to revisiting this?

[erenon]

Thanks for taking a look. The goal is to make this tiny library as broadly and effortlessly useful as possible, let's find a solution.

A possible change would be:

• Run clang-tidy on each header in hdrs (this PR)
• Remove HeaderFilterRegex: ".*" from .clang-tidy

My only remaining concern is around performance. Given a target (foo.cpp, foo.h), with this change, we'd check foo.h twice. This argument is weakened by the fact that if there are more source files including the header, the header is checked even more times.

Optimal would be if clang-tidy didn't waste much resources on ignored headers: I'll need to check, If that's the case, I have no further objections, and this can be merged (with conflicts fixed).

[erenon]

Fixed in https://github.com/erenon/bazel_clang_tidy/pull/56 (conflict resolution and a separate change was needed). If that works for you, I'll close this. Thanks for the effort.