[PATCH] D23130: [Clang-tidy] Add a check for definitions in the global namespace.

[Benjamin Kramer via cfe-commits]

bkramer added a comment.

In https://reviews.llvm.org/D23130#505290, @alexfh wrote:

> 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 :(


================
Comment at: clang-tidy/misc/GlobalNamespaceCheck.cpp:46
@@ +45,3 @@
+    // extern "C" globals need to be in the global namespace.
+    if (VDecl->isExternC())
+      return;
----------------
Prazek wrote:
> 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
I'll fix this once we figured out where to put this check.

================
Comment at: clang-tidy/misc/GlobalNamespaceCheck.cpp:67
@@ +66,3 @@
+  }
+
+  // FIXME: If we want to be really fancy we could move declaration-less
----------------
Prazek wrote:
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D23130