modernize-use-using: don't change code in extern "C" {}

Quuxplusone commented 6 years ago


Bugzilla Link	PR35924
Status	NEW
Importance	P normal
Reported by	Duncan Thomson (dthomson@esri.com)
Reported on	2018-01-12 03:40:13 -0800
Last modified on	2021-10-27 09:46:24 -0700
Version	unspecified
Hardware	PC Linux
CC	alexfh@google.com, dank@deadmime.org, development@jonas-toth.eu, djasper@google.com, eugene.zelenko@gmail.com, george.burgess.iv@gmail.com, hokein@google.com, john@mcfarlane.name, klimek@google.com, kryshaczek@gmail.com, legalize@xmission.com, prazek@google.com, Tyker1@outlook.com, viniciustinti@gmail.com
Fixed by commit(s)
Attachments	`use-using.patch` (2204 bytes, text/plain) `pr35924-patches.tgz` (54831 bytes, application/gzip)
Blocks
Blocked by
See also

clang-tidy generates warnings specific to C++ for code that lies within the
scope of an extern "C" {}

E.g. consider the following code

/* example.cpp */

/* This block simulates a C header */
#ifdef __cplusplus
extern "C" {
#endif

typedef int my_int_t;

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

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

int main()
{
  my_int_t value{42};
  std::cout << value << std::endl;
  return 0;
}

Compile with:

$ clang++ --std=c++14 -Wall example.cpp -o example

Run clang-tidy as:

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

gives:

5307 warnings generated.
/home/duncan/temp/example.cpp:5:1: warning: use 'using' instead of 'typedef'
[modernize-use-using]
typedef int my_int_t;
^~~~~~~~~~~~~~~~~~~~~
using my_int_t = int
Suppressed 5306 warnings (5306 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.

I don't think C++ checks should be applied to code within the scope of an
extern "C" block.

Quuxplusone commented 6 years ago

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.

Quuxplusone commented 6 years ago

Attached use-using.patch (2204 bytes, text/plain): Ignore entries when the MatcheDecl is ExternCContext

Quuxplusone commented 6 years ago

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

Quuxplusone commented 6 years ago

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 <iostream>

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
         ^~~~~~~~~~
         <cstdlib>
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.

Quuxplusone commented 6 years ago

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.

Quuxplusone commented 4 years ago

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?

Quuxplusone commented 4 years ago

Hi George.

Could you talk more about your thoughts?

Quuxplusone commented 4 years ago

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.

Quuxplusone commented 4 years ago

Do share please.

Quuxplusone commented 4 years ago

Attached pr35924-patches.tgz (54831 bytes, application/gzip): patch bundle to supplement comment 8

Quuxplusone commented 4 years ago

Thanks for sharing.

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