bkramer marked 2 inline comments as done.
bkramer added a comment.
In https://reviews.llvm.org/D23130#589643, @alexfh wrote:
> > 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/
================
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:77
+ }
+ diag(
+ D->getLocStart(),
----------------
aaron.ballman wrote:
> Is this formatting that clang-format generates?
Yes. I clang-formatted the entire file.
================
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:
> 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.
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.
================
Comment at: test/clang-tidy/google-global-names.cpp:18
+extern "C" void g();
+extern "C" void h() {}
+
----------------
aaron.ballman wrote:
> 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.
Test added, doesn't diagnose.
https://reviews.llvm.org/D23130
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
