Page MenuHomePhabricator

[Clang-tidy] Add a check for definitions in the global namespace.
AcceptedPublic

Authored by bkramer on Aug 3 2016, 12:54 PM.

Details

Summary

This is prone to ODR violations and generally frowned upon in many
codebases (e.g. LLVM). The checker flags definitions, variables and
classes in the global namespace. Common false positives like extern "C"
functions are filtered, symbols with internal linkage or hidden
visibility are not warned on either.

On LLVM most of the instances are helper functions that should be just
made static or globals that belong into a namespace.

Diff Detail

Event Timeline

bkramer updated this revision to Diff 66696.Aug 3 2016, 12:54 PM
bkramer retitled this revision from to Add a check for definitions in the global namespace..
bkramer updated this object.
bkramer added a reviewer: alexfh.
bkramer added a subscriber: cfe-commits.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Eugene.Zelenko retitled this revision from Add a check for definitions in the global namespace. to [Clang-tidy] Add a check for definitions in the global namespace..Aug 3 2016, 1:26 PM
Eugene.Zelenko set the repository for this revision to rL LLVM.
bkramer updated this revision to Diff 66716.Aug 3 2016, 2:13 PM

Add relnote.

Eugene.Zelenko set the repository for this revision to rL LLVM.Aug 3 2016, 2:16 PM
alexfh edited edge metadata.Aug 3 2016, 2:17 PM

Do we want to merge this check with google/GlobalNamesInHeadersCheck.cpp that targets a similar set of issues?

DefinitionsInHeaders is tackling a different problem. IMO DefinitionsInHeaders is something that should be on by default everywhere, while this check for definitions in the global namespace is more of a coding style issue.

GlobalNamesInHeaders is a bit of a misnomer, it looks for using declarations in headers. I don't think it makes sense to merge this check into it either, with the new check we mostly find things exported by accident in .cpp files, it has little to do with headers.

I'm welcome to naming suggestions for this check, btw ;)

alexfh added a comment.Aug 3 2016, 4:00 PM

DefinitionsInHeaders is tackling a different problem. IMO DefinitionsInHeaders is something that should be on by default everywhere, while this check for definitions in the global namespace is more of a coding style issue.

Yes, DefinitionsInHeaders is a different beast.

GlobalNamesInHeaders is a bit of a misnomer, it looks for using declarations in headers.

The google-global-names-in-headers check aims to "flag global namespace pollution in header files" including declarations of classes, functions and other entities. However, "right now it only triggers on using declarations and directives", so it's basically under-implemented. The only problem, I guess, is keeping false positives under control. The check you're proposing is somewhat close. It isn't specifically limited to headers (the google-global-names-in-headers check can run on all files if properly configured, and we can rename it to remove the -in-headers part). And the second difference is that it is limited to definitions, but I don't yet understand, why it is important, since declarations in the global namespace (except for using declarations, typedefs, etc.) will have a corresponding definition in the global namespace. I guess, these checks, if somewhat different, may be similar enough to share some implementation.

Prazek added inline comments.Aug 3 2016, 8:45 PM
clang-tidy/misc/GlobalNamespaceCheck.cpp
46 ↗(On Diff #66716)

I think it would be better to check it in matcher.
I see that there is isExternC, but it only works for FunctionDecl, but I think that adding isExternC for VarDecl would be good

67 ↗(On Diff #66716)

What about macros? I think you should check isMacroId on location here (don't do fixit when it is in macro, but print warning)

Also please add tests for it.

And the second difference is that it is limited to definitions, but I don't yet understand, why it is important, since declarations in the global namespace (except for using declarations, typedefs, etc.) will have a corresponding definition in the global namespace. I guess, these checks, if somewhat different, may be similar enough to share some implementation.

My reasoning was to avoid duplicated warnings (on both declaration and definition) and false positives on headers importing C symbols. I think that the latter would trigger an explosion in false positives :(

clang-tidy/misc/GlobalNamespaceCheck.cpp
46 ↗(On Diff #66716)

I'll fix this once we figured out where to put this check.

67 ↗(On Diff #66716)

The FixIt isn't actually implemented. Adding FixIts for this check is something I'm planning to do, but it's not part of this patch and doesn't make sense to implement before we know that this check is actually working properly.

aaron.ballman added inline comments.Aug 4 2016, 5:39 AM
clang-tidy/misc/GlobalNamespaceCheck.cpp
72 ↗(On Diff #66716)

ODR violations instead of ODR conflicts?

docs/clang-tidy/checks/misc-global-namespace.rst
7 ↗(On Diff #66716)

violations instead of conflicts. Also, expanding the documentation a bit more would be useful -- not everyone understands what an ODR violation is, why it would be bad, etc. It can even be as simple as adding in an external link for more info.

test/clang-tidy/misc-global-namespace.cpp
13 ↗(On Diff #66716)

Add a case for extern "C" int i;?

Also, isn't this a definition with external linkage in C++ (or is it only in C)? extern int i = 30; Assuming it's a definition, should this also trigger the diagnostic?

bkramer updated this revision to Diff 66792.Aug 4 2016, 5:47 AM
bkramer edited edge metadata.
  • Merge google-global-names-in-headers and misc-global-namespace into a new check google-global-names.
alexfh added a comment.Aug 4 2016, 6:53 AM

Could you run the check on LLVM and post a summary of results?

clang-tidy/google/GlobalNamesCheck.cpp
91

Is this already filtered-out by the matcher?

115

The warning explains, what's wrong, but doesn't tell why. We don't need the level of details of the documentation here, but maybe expand the message a bit to cover some of the possible effects (e.g. "this can cause ODR violations" or something like this)?

docs/ReleaseNotes.rst
62

Please mention the old check name as well.

docs/clang-tidy/checks/google-global-names.rst
3–4

Cut extra =s.

bkramer updated this revision to Diff 66800.Aug 4 2016, 7:08 AM

Address review comments.

Could you run the check on LLVM and post a summary of results?

I have a full list at https://reviews.llvm.org/P7085. It's huge.

For cases where the external linkage is desired, how would you go about silencing this diagnostic?

test/clang-tidy/google-global-names.cpp
13–14

This strikes me as being intentional enough to warrant not diagnosing because of the extern keyword.

bkramer added inline comments.Aug 4 2016, 7:44 AM
test/clang-tidy/google-global-names.cpp
13–14

The only case I see where this pattern is valuable is interfacing with C code. Not sure yet if we want to allow that or enforce extern "C" instead. Ideas?

an extern global in the global namespace still feels like something we should warn on :|

aaron.ballman added inline comments.Aug 4 2016, 7:58 AM
test/clang-tidy/google-global-names.cpp
13–14

Yet externs in the global namespace do happen for valid reasons (such as not breaking ABIs by putting the extern definition into a namespace or changing the language linkage) -- I'm trying to think of ways we can allow the user to silence this diagnostic in those cases. I feel like in cases where the user writes "extern", they're explicitly specifying their intent and that doesn't seem like a case to warn them about, in some regards. It would give us two ways to silence the diagnostic (well, three, but two are morally close enough):

  1. Put it into a namespace
  2. Slap extern on it if it is global for C++ compatibility (such as ABIs)
  3. Slap extern "C" on it if it global for C compatibility

I suppose we could require extern "C++" instead of extern, but I don't think that's a particularly common use of the language linkage specifier?

alexfh added a comment.Nov 7 2016, 2:31 PM

Benjamin, what's the plan here?

bkramer updated this revision to Diff 77216.Nov 8 2016, 10:48 AM

Update to head

Benjamin, what's the plan here?

I still think this check is useful, particularly for LLVM. I also don't think any of the existing review comments still apply or have ever applied in the first place, so I rebased this onto head, it's up for review again.

aaron.ballman added inline comments.Nov 8 2016, 11:21 AM
clang-tidy/google/GlobalNamesCheck.cpp
78

Is this formatting that clang-format generates?

test/clang-tidy/google-global-names.cpp
13–14

I still think that a user explicitly writing 'extern' is expecting external linkage and all that goes along with it.

19

Can you also add a test:

extern "C++" void h2() {}

I believe this will diagnose, but whether it should diagnose or not, I'm less certain of.

and generally frowned upon in many codebases (e.g. LLVM)

Should it still be a part of google/? The old check was enforcing a part of the Google C++ style guide, but the new one seems to be somewhat broader. Am I mistaken?

bkramer updated this revision to Diff 77410.Nov 9 2016, 3:31 PM

Add extern "C++" test case.

bkramer marked 2 inline comments as done.Nov 9 2016, 3:34 PM

and generally frowned upon in many codebases (e.g. LLVM)

Should it still be a part of google/? The old check was enforcing a part of the Google C++ style guide, but the new one seems to be somewhat broader. Am I mistaken?

The google style guide has a rule that code should usually be placed in a namespace. The wording there is fuzzy, but I believe this check still belongs under google/

clang-tidy/google/GlobalNamesCheck.cpp
78

Yes. I clang-formatted the entire file.

test/clang-tidy/google-global-names.cpp
13–14

I disagree. If this is a special variable to be accessed via dlopen it should be extern "C". If not it should be in a namespace.

19

Test added, doesn't diagnose.

malcolm.parsons added inline comments.
clang-tidy/google/GlobalNamesCheck.cpp
97

Should isMSVCRTEntryPoint() be checked too?

aaron.ballman added inline comments.Nov 10 2016, 6:40 AM
test/clang-tidy/google-global-names.cpp
13–14

I'm thinking more that it's not a special variable to be accessed via dlopen, but instead is a special implementation detail that doesn't need to be exposed via a header file. For instance, see ClangTidyMain.cpp. Such uses have no way to silence this check, which may be fine since it's in the Google module. I merely wanted to point it out because it does happen for valid reasons, and having a way to silence a diagnostic aside from NOLINT is nice.

bkramer marked 2 inline comments as done.Nov 16 2016, 6:15 AM
bkramer added inline comments.
test/clang-tidy/google-global-names.cpp
13–14

That's an example of something that should be in a namespace. Currently it's polluting the global namespace which will break anyone who happens to use the same name for a symbol anywhere else. I think that flagging those cases is useful.

aaron.ballman added inline comments.
clang-tidy/google/GlobalNamesCheck.cpp
63

s/unless/Unless

85

I wonder if this check would have better performance by making this a local AST matcher instead? UsingDecl and UsingDirectiveDecl do not have linkage, so it may require adjusting the matcher somewhat.

97

Yes, it should.

clang-tidy/google/GoogleTidyModule.cpp
68

Given that this was shipped under the old name, I think we need to figure out our policy for how to handle this. It also comes up in D26511, so I would like us to be consistent with what we do.

test/clang-tidy/google-global-names.cpp
13–14

Okay, that's fair. I was thinking that since it was in the global namespace, it was there for a reason. After looking a bit more closely, I think you're correct, this would be a true-positive. You've convinced me and I retract my concerns. :-)

bkramer updated this revision to Diff 80574.Dec 7 2016, 4:56 AM
bkramer marked 9 inline comments as done.
  • Moved external linkage check to matcher
  • added msvcrt entry point check
  • fixed comment typos.
bkramer added inline comments.Dec 7 2016, 4:56 AM
clang-tidy/google/GlobalNamesCheck.cpp
91

Nope.

clang-tidy/google/GoogleTidyModule.cpp
68

There was no special handling in D26511, so I left this as-is.

malcolm.parsons added inline comments.Dec 7 2016, 5:01 AM
clang-tidy/google/GoogleTidyModule.cpp
68

D26511 made the existing check documentation redirect to the new documentation.

alexfh accepted this revision.Mar 14 2018, 9:15 AM

It looks like all concerns were resolved.

LG, if you still want to proceed with this patch.

This revision is now accepted and ready to land.Mar 14 2018, 9:15 AM

I'd like to, but I don't know when I find time to rebase this thing after more than a year of waiting for review.

I'd like to, but I don't know when I find time to rebase this thing after more than a year of waiting for review.

Sorry, it was just lost ¯\_(ツ)_/¯. I may be relying on pings from the other side too much.