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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
