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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:77
+    }
+    diag(
+        D->getLocStart(),
----------------
Is this formatting that clang-format generates?


================
Comment at: test/clang-tidy/google-global-names.cpp:13-14
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'i' declared in the global namespace
+extern int ii = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'ii' declared in the global namespace
+
----------------
aaron.ballman wrote:
> bkramer wrote:
> > aaron.ballman wrote:
> > > This strikes me as being intentional enough to warrant not diagnosing because of the `extern` keyword.
> > 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 :|
> 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?
I still think that a user explicitly writing 'extern' is expecting external linkage and all that goes along with it.


================
Comment at: test/clang-tidy/google-global-names.cpp:18
+extern "C" void g();
+extern "C" void h() {}
+
----------------
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.


https://reviews.llvm.org/D23130