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

Aaron Ballman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 6 11:36:13 PDT 2019


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:
> > 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?


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

https://reviews.llvm.org/D33841





More information about the llvm-commits mailing list