[PATCH] D33841: [clang-tidy] redundant keyword check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: test/clang-tidy/readability-redundant-extern.cpp:37
+
+void another_file_scope(int _extern);
----------------
koldaniel wrote:
> aaron.ballman wrote:
> > koldaniel wrote:
> > > aaron.ballman wrote:
> > > > More tests that I figured out:
> > > > ```
> > > > namespace {
> > > > extern void f(); // 'extern' is not redundant
> > > > }
> > > > 
> > > > namespace a {
> > > > namespace {
> > > > namespace b {
> > > > extern void f(); // 'extern' is not redundant
> > > > }
> > > > }
> > > > }
> > > > 
> > > > // Note, the above are consequences of http://eel.is/c++draft/basic.link#6
> > > > 
> > > > #define FOO_EXTERN extern
> > > > typedef int extern_int;
> > > > 
> > > > extern_int FOO_EXTERN foo(); // 'extern' is redundant, but hopefully we don't try to fixit this to be '_int FOO_EXTERN foo();'
> > > > 
> > > > // The above is a weird consequence of how specifiers are parsed in C and C++
> > > > ```
> > > In the first two examples extern is redundant:
> > > 
> > > "An unnamed namespace or a namespace declared directly or indirectly within an unnamed namespace has internal linkage. All other namespaces have external linkage."
> > > 
> > > Also, based on the examples in http://eel.is/c++draft/basic.link#8 , the extern keyword has no effect in case of unnamed namespaces. In case of 'C' linkage defined by an extern keyword the checker does not warn.
> > > 
> > > In the first two examples extern is redundant:
> > 
> > The `extern` keyword there isn't redundant, it's useless. When I hear "redundant", I read it as "you can remove this keyword because the declaration is already `extern`" but that's not the case there.
> > 
> > You are correct that the declarations retain internal linkage -- that's why I think the `extern` keyword isn't redundant so much as it is confusing. We already issue a diagnostic for this in the frontend, so I think we may just want to silence the diagnostic here entirely. https://godbolt.org/z/WE6Fkd
> > 
> > WDYT?
> I see, you are right, calling it redundant is not the best way to describe the situation. What I wanted to achieve here  is that I wanted this checker to warn on every occurrence of the keyword extern when it seems to be useless (redundant, unnecessary, etc.). If there are other checkers which warn in situations like that (i.e. when extern has no effect), we could silence these warnings (or we could handle this checker as a more generic one which warns not on redundant but on unnecessary uses of extern).
I think it is okay to keep the check as-is but change the wording of the diagnostic slightly. Maybe `"unnecessary 'extern' keyword; %select{the declaration already has external linkage|the 'extern' specifier has no effect}0"` to really make it clear what's going on?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D33841/new/

https://reviews.llvm.org/D33841