bkramer marked 2 inline comments as done. bkramer added inline comments.
================ 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: > > > 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. https://reviews.llvm.org/D23130 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
