[clang-tidy] Undesired diagnostics due to macro expansion from third-party headers

Hi All,

I would like to know people’s opinion about clang-tidy diagnostics due to macro expansion of the macro defined in a third-party header. It is a huge pain for clang-tidy deployment on a large codebase that actively uses third-party bits that might not use the same clang-tidy checks enabled. Inserting NOLINT annotation might not be a solution because in some cases it is important to keep third-party code unmodified at all. Just one example from a very popular gflags library. This library requires using macro in use code like this:

DEFINE_bool(some_bool_flag,
            "the default value should go here, not the description",
            false);

But the macro has an outdated version of compiler assert that uses C-style arrays. Therefore use code becomes incompatible with clang-tidy check modernize-avoid-c-array. Another example of problematic is googletest/googlemock with lots of macro that you cannot avoid.

I created RFC diff https://reviews.llvm.org/D90835 but got very limited feedback about the proposed approach. So bringing it here to increase visibility of the problem. Please share what you think about the problem itself and how it should be addressed in general. Feedback about the diff is also very appreciated but first of all I would like to understand how you think the problem should be addressed in principle.

Thanks,
Dmitry

Hi All,

I would like to know people's opinion about clang-tidy diagnostics due to macro expansion of the macro defined in a third-party header. It is a huge pain for clang-tidy deployment on a large codebase that actively uses third-party bits that might not use the same clang-tidy checks enabled. Inserting NOLINT annotation might not be a solution because in some cases it is important to keep third-party code unmodified at all. Just one example from a very popular gflags library. This library requires using macro in use code like this:

DEFINE_bool(some_bool_flag,
            "the default value should go here, not the description",
            false);

But the macro has an outdated version of compiler assert that uses [C-style arrays](https://github.com/gflags/gflags/blob/2e227c3daae2ea8899f49858a23f3d318ea39b57/src/gflags.h.in#L528). Therefore use code becomes incompatible with clang-tidy check modernize-avoid-c-array. Another example of problematic is googletest/googlemock with lots of macro that you cannot avoid.

I created RFC diff ⚙ D90835 [clang-tidy] Ignore diagnostics due to macro expansion from not-interested headers but got very limited feedback about the proposed approach. So bringing it here to increase visibility of the problem. Please share what you think about the problem itself and how it should be addressed in general. Feedback about the diff is also very appreciated but first of all I would like to understand how you think the problem should be addressed in principle.

Thank you for raising the concern! In general, I think these sorts of
diagnostics involving macro expansions are difficult. Consider:

#define MACRO(x) ({int foo[12] = {(int)x}; foo[0]; }) // In some
third-party header
MACRO(0); // In some user code

In this case, the user has no control over the definition of MACRO and
they have no way to silence the warning from the header, so they may
not appreciate the diagnostic. However, something like:

#define MACRO(x) x // In some third-party header
MACRO(int foo[12] = {0}); // In some user code

is something that the user has control over and may reasonably want to
get diagnostics about. Clang-tidy is not supposed to diagnose things
in header files unless the header file is listed in --header-filter
(according to the public docs in
Clang-Tidy — Extra Clang Tools 18.0.0git documentation) but it seems that
modernize-avoid-c-arrays is diagnosing the macro expansion location,
even when the problematic code is within the macro definition itself
(Compiler Explorer) and that may be worth seeing if we
can improve. I'd expect the diagnostic for MACRO1 but not MACRO2 in an
ideal world.

~Aaron

Thank you for the detailed response!

It is possible to distinguish locations from macro arguments and macro bodies. It is not implemented in my diff but it is a straightforward addition to report the issue for the second case (macro argument) and don’t report for the first case (macro body). But in general the exact point for reporting issues is somewhat arbitrary i.e. it is the most convenient place for the user to see what is wrong or about which declaration the report is. This report point can come from the macro argument but the rest where the issue really is could come from the macro body. In case of modernize-avoid-c-arrays, the point of the report is the beginning of the whole declaration. But the issue could be reported on the name of the variable or the array type, they are also reasonable points for the report. So there is no one solution that will work the best in all cases. But the issue is important and I think clang-tidy needs to have some options to hide the majority of the unwanted reports from macro expansions. Implementing this logic in every check is not practical so I think it should be common logic for all checks like NOLINT.

Dmitry