The exclusion of extern "C" blocks is done for each check independently. This bug seems to refer to modernize-use-using. If you have other examples, please report them as well.

Ignore entries when the MatcheDecl is ExternCContext
This patch may not apply cleanly to HEAD, as we're using a local version in a monorepo.

Anyways, we've applied the following change:

• When the MatchedDecl identifies itself as being in an externCContext, just return
• Added in a test case to verify when some types are in an extern C

What needs to be done to get more traction on resolving this issue?

modernize-use-using isn't the only check to have the issue, modernize-deprecated-headers and modernize-use-nullptr have it too.

example:

/* This block simulates a C header */

#ifdef __cplusplus
extern "C" {
#endif

#include <stdlib.h> // for NULL

int* A = NULL;

#ifdef __cplusplus
} // extern "C"
#endif

// This block simulates a C++ implementation/use of the C header's API
#include

int main()
{
std::cout << A << std::endl;
return 0;
}

Run clang-tidy as:

$ clang-tidy test.cpp -checks='*' -- -std=c++14 -Wall

output:
15925 warnings generated.
test.cpp:7:10: warning: inclusion of deprecated C++ header 'stdlib.h'; consider using 'cstdlib' instead [modernize-deprecated-headers]
#include <stdlib.h> // for NULL
^~~~~~~~~~

test.cpp:9:10: warning: use nullptr [modernize-use-nullptr]
int* A = NULL;
^~~~
nullptr
Suppressed 15922 warnings (15922 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Probably all checks have this issue, as its very uncommon to check for the extern "C"/extern "C++" constructs, as the language-options for the translation unit is just assumes everywhere.

It would be best, to have something central and not do that process manually for every single matcher, as that would be cumbersome, error-prone and not scalable.

So I've been thinking about this problem a bit, and wanted to dump what I've got so far + see if I can revive the discussion here.

Ideally, what we want here is knowledge of whether code has to be parseable as C or not. Lacking a -c-header-filter= (and the will of users to populate that accurately), I agree our best bet is a bundle o' heuristics, which will probably lead to tidy being over-conservative or over-aggressive in some cases:

extern "C" inline void foo() {
char cs[4];
// modernize-loop-convert says we should transform this. Can we? Who knows.
for (unsigned i = 0; i < sizeof(cs); ++i)
bar(&cs[i]);
}

The most straightforward way I can see to do that would be to get all tidy checks to declare their C-compatibility somehow, and have the matchers of C-incompatible checks implicitly wrapped in essentially unless(Matcher, probablyHasToBeSourceCompatibleWithC()) by default. For checks that want to control their own destiny, we can add the capability to opt-in/opt-out per matcher, but the default will depend on the check's general C compatibility.

I have an idea of how to do the above with a few (unfortunately) broad refactors. Before I dump a wall o' words describing that, does anyone have thoughts on this approach at a high level?

Hi George.

Could you talk more about your thoughts?

Right -- I think I can summarize in a small fence o' words, instead of a wall of them ;)

IIRC, my idea involved essentially two broad steps:

• converting all checks (e.g., ProBoundsPointerArithmeticCheck) that check LangOpts in registerMatchers to use isLanguageVersionSupported instead
• making isLanguageVersionSupported return something in terms of compatible languages/versions, instead of "yeah, this LangOpts LGTM." The simplest thing would be returning a struct { bool isEnabledForCurrentLangOpts; bool isCCompatible; }. This'd probably also need a rename of this method.

After these, we'd end up with per-check info about whether a given check provides C-compatible diagnostics. The second step would (unfortunately) break the build of any downstream checks which use override on isLanguageVersionSupported, but it should be a super simple cleanup.

If we could then funnel this new isCCompatible bit into clang-tidy's core matching logic somehow (waves hands), we could selectively disable checks if tidy thinks we're in a context in which doing so is necessary (e.g., in an extern "C" block).

I think I got most of the refactor I mentioned done, but didn't put time into the "making the matching logic make use of this information" part. Happy to share unfinished patches on an old version of tidy if they'd be of use.

Do share please.

patch bundle to supplement comment 8
Attached is a tgz containing the series of commits I have so far. It's all very much a WIP that I planned to rebase -i into something more sensible at some point. ;) Dev was done on top of 395e2c0.

Glancing at them, apparently I felt the need to do a refactor of registerMatchers(ast_matchers::MatchFinder *Finder) -> registerMatchers(ast_matchers::MatchRegistrar *Finder). My best guess for why is that I was planning on making MatchFinder::addMatcher take a IsCCompatible bit (or perhaps some kind of const LanguageCompatibility & thing, depending on whether anyone thought we'd see another variant of this kind of thing pop up). Probably seemed more clean to do that at the time than adding a void setNewlyAddedChecksCCompatibility(bool) to MatchFinder, and we likely don't want all checks to have to pass this extra info when registering their checks.

Standard disclaimer: this was all experimental code. No guarantees that it'll translate directly into a solution for this bug, or that it's suitable to try to upstream into LLVM without polish. :)

Thanks for sharing.

Another patch is taking a bit more time than I expected.
I will be back on this soon.

*** Bug llvm/llvm-bugzilla-archive#49181 has been marked as a duplicate of this bug. ***

If you have other examples, please report them as well.

modernize-use-trailing-return-type has this problem.

a.cpp:

extern "C" {
int f();
}

Console:

$ clang-tidy --version
LLVM (http://llvm.org/):
LLVM version 11.1.0

Optimized build.
Default target: x86_64-pc-linux-gnu
Host CPU: broadwell

$ clang-tidy a.cpp -checks=modernize-use-trailing-return-type
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "a.cpp"
No compilation database found in /home/jmcfarla/eg or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or
directory
json-compilation-database: Error while opening JSON database: No such file or
directory
Running without flags.
1 warning generated.
/home/jmcfarla/eg/a.cpp:2:5: warning: use a trailing return type for this
function [modernize-use-trailing-return-type]
int f();

auto    -> int

Another check: cppcoreguidelines-pro-bounds-constant-array-index

I count five so far (excluding aliases):

• modernize-deprecated-headers
• modernize-use-using
• modernize-use-trailing-return-type
• cppcoreguidelines-pro-bounds-constant-array-index
• modernize-use-nullptr

I've started a menagerie (https://godbolt.org/z/4ss8bGjzr). It triggers the checks for which we might consider setting IsCCompatible=false. It does so for Clang 14.0.0git.

• cppcoreguidelines-pro-type-vararg

https://godbolt.org/z/WMP4TqTE4

• cppcoreguidelines-pro-bounds-array-to-pointer-decay

recommends using span as an alternative.

https://godbolt.org/z/xc1zrGdjM

(LMK if these are not helpful. Updating as I discover them.)

• cppcoreguidelines-macro-usage

https://godbolt.org/z/b3ro961qh

mentioned in issue llvm/llvm-bugzilla-archive#49181

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Assign the issue to you.
2. Fix the issue locally.
3. Run the test suite locally.
   3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can
   e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a git commit
5. Run git clang-format HEAD~1 to format your changes.
6. Submit the patch to Phabricator.
   6.1) Detailed instructions can be found here

For more instructions on how to submit a patch to LLVM, see our documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvm/issue-subscribers-good-first-issue

@Endilll Could this be assigned to me

@Da-Viper sure!