aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added inline comments.
================
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:62
+ Result.SourceManager->getExpansionLoc(D->getLocStart()))) {
+ // unless that file is a header.
+ if (!utils::isSpellingLocInHeaderFile(
----------------
s/unless/Unless
================
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:84
+ // Ignore decls with internal linkage.
+ if (!D->isExternallyVisible())
+ return;
----------------
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.
================
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:96
+ // main() should be in the global namespace.
+ if (FDecl->isMain())
+ return;
----------------
malcolm.parsons wrote:
> Should `isMSVCRTEntryPoint()` be checked too?
Yes, it should.
================
Comment at: clang-tidy/google/GoogleTidyModule.cpp:68
+ CheckFactories.registerCheck<readability::GlobalNamesCheck>(
+ "google-global-names");
CheckFactories.registerCheck<clang::tidy::readability::FunctionSizeCheck>(
----------------
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.
================
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
+
----------------
bkramer wrote:
> aaron.ballman wrote:
> > bkramer wrote:
> > > 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.
> > 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.
> 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.
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. :-)
https://reviews.llvm.org/D23130
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
