Page MenuHomePhabricator

[clang-tidy] redundant 'extern' keyword check
Needs ReviewPublic

Authored by koldaniel on Jun 2 2017, 10:26 AM.

Details

Summary

finds and removes redundant 'extern' keywords

Diff Detail

Event Timeline

SilverGeri created this revision.Jun 2 2017, 10:26 AM
mgehre added a subscriber: mgehre.Jun 2 2017, 10:49 AM

Please have a look at the discussion in https://reviews.llvm.org/D18914 (which is for a similar check)
It turns out that an inline specifier which is redundant for deciding linkage is still
used to drive the inliner. Basically, the inliner will consider larger function bodies for inlining
if the have an explicit inline specifier.

Eugene.Zelenko retitled this revision from redundant keyword check to [clang-tidy] redundant keyword check.Jun 2 2017, 4:23 PM
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a project: Restricted Project.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/readability-redundant-keyword.rst
6 ↗(On Diff #101234)

checker -> check. Please use `` to highlight language constructs here and below.

13 ↗(On Diff #101234)

Unnecessary empty line.

20 ↗(On Diff #101234)

May be three dots will be better as common punctuation?

22 ↗(On Diff #101234)

Please add newline.

Prazek added a comment.Jun 2 2017, 4:31 PM

Feature request: removing "void" from int main(void)

Feature request: removing "void" from int main(void)

This will duplicate modernize-redundant-void-arg.

Prazek added a comment.Jun 2 2017, 4:34 PM

extern on function definition is also redundant, right?

Also, what about:
inline extern void foo();
(you check if it startswith extern)

Prazek removed a subscriber: Prazek.Jun 2 2017, 4:34 PM
alexfh requested changes to this revision.Jun 7 2017, 12:43 PM
alexfh added inline comments.
docs/clang-tidy/checks/readability-redundant-keyword.rst
8 ↗(On Diff #101234)

Could you explain, why you think extern is redundant in function declarations?

This revision now requires changes to proceed.Jun 7 2017, 12:43 PM
aaron.ballman added inline comments.Jun 14 2017, 4:08 PM
clang-tidy/readability/RedundantKeywordCheck.cpp
22 ↗(On Diff #101234)

Why do you need to do a textual search that the first token in the declaration is extern or inline? That seems like it would fail on common cases that you would expect to catch:

#define FOO_EXTERN extern

FOO_EXTERN void blah();
test/clang-tidy/readability-redundant-keyword.cpp
22 ↗(On Diff #101234)

Why is this okay, but the following is not?

extern "C++" void ok2();
xazax.hun added inline comments.Jun 22 2017, 4:22 AM
docs/clang-tidy/checks/readability-redundant-keyword.rst
8 ↗(On Diff #101234)

Just to be clear here, do you think there is a case where extern is not redundant or you just want the documentation to be extended?

alexfh added inline comments.Jul 11 2017, 7:26 AM
docs/clang-tidy/checks/readability-redundant-keyword.rst
8 ↗(On Diff #101234)

Sorry for being unclear. I would expect a more in-depth explanation of why the keyword is redundant with references to the appropriate sections of the standard or some other authoritative source.

koldaniel commandeered this revision.Jun 7 2018, 7:31 AM
koldaniel added a reviewer: SilverGeri.
koldaniel marked an inline comment as done.Jan 29 2019, 9:34 AM
koldaniel added inline comments.
docs/clang-tidy/checks/readability-redundant-keyword.rst
8 ↗(On Diff #101234)

https://en.cppreference.com/w/cpp/language/language_linkage

The default language linkage is C++, so without any additional parameters it is redundant (extern "C++" can also be redundant, but it depends on the context). In C context (extern "C") the situation is the same, extern keyword is redundant for function declarations (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf - 6.2.2.5)

koldaniel updated this revision to Diff 184111.Jan 29 2019, 9:40 AM

As it was mentioned earlier, I think it would be a better way forward to handle the check of redundant inlines in the scope of the other checker: https://reviews.llvm.org/D18914

Please mention new check in Release Notes.

clang-tidy/readability/RedundantExternCheck.cpp
61

Please add new line.

clang-tidy/readability/RedundantExternCheck.h
6

Please update license statement. See current Clang-tidy code as example. Same in other files.

docs/clang-tidy/checks/readability-redundant-extern.rst
7

Please remove This checker. Language constructs should be enclosed in ``.

MyDeveloperDay added inline comments.
clang-tidy/readability/RedundantExternCheck.cpp
46

current convention would be that this should be Offset

aaron.ballman added inline comments.Feb 11 2019, 6:07 AM
docs/clang-tidy/checks/readability-redundant-extern.rst
5

The underlining here is too long.

16

Please add the newline to the end of the file.

test/clang-tidy/readability-redundant-extern.cpp
3

This is missing some other negative tests, like a file-scope:

void file_scope();

an especially interesting test would be:

void another_file_scope(int _extern);
aaron.ballman added inline comments.Mar 14 2019, 11:29 AM
clang-tidy/readability/RedundantExternCheck.cpp
28

Formatting is incorrect (elsewhere too). You should run the patch through clang-format.

33

FD->getStorageClass() != SC_Extern

36

How about "redundant 'extern' storage class specifier"?

test/clang-tidy/readability-redundant-extern.cpp
38

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++
alexfh added inline comments.Apr 3 2019, 7:14 AM
docs/clang-tidy/checks/readability-redundant-keyword.rst
8 ↗(On Diff #101234)

This sort of a description would be helpful in the documentation of the check. Users are not likely to search for clarifications in code review comments on LLVM Phabricator ;)

koldaniel marked an inline comment as done.May 5 2019, 12:11 PM
koldaniel added inline comments.
test/clang-tidy/readability-redundant-extern.cpp
38

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.

koldaniel updated this revision to Diff 198194.May 5 2019, 12:15 PM

Typedef and macro maches fixed, new test cases added.

aaron.ballman added inline comments.May 6 2019, 11:36 AM
test/clang-tidy/readability-redundant-extern.cpp
38

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?

koldaniel marked an inline comment as done.May 6 2019, 1:33 PM
koldaniel added inline comments.
test/clang-tidy/readability-redundant-extern.cpp
38

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).

aaron.ballman added inline comments.May 7 2019, 6:36 AM
test/clang-tidy/readability-redundant-extern.cpp
38

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?

koldaniel updated this revision to Diff 203154.Jun 5 2019, 7:45 AM

Updating warnings.

lebedev.ri retitled this revision from [clang-tidy] redundant keyword check to [clang-tidy] redundant 'extern' keyword check.Jun 5 2019, 9:04 AM
lebedev.ri edited the summary of this revision. (Show Details)
lebedev.ri added inline comments.
clang-tidy/readability/RedundantExternCheck.cpp
31

Can you do that in registerMatchers()?

44–45

Similarly, do this in registerMatchers()

50

StringLiteral Extern = StringLiteral("extern");

51

Extern

54

Extern.size()

60

Extern

docs/clang-tidy/checks/readability-redundant-extern.rst
11

Please 80-char wrap

15

More examples? A namespace one?

koldaniel marked an inline comment as done.Jun 12 2019, 9:56 AM
koldaniel added inline comments.
clang-tidy/readability/RedundantExternCheck.cpp
31

The only way I found for that is to use the matcher hasExternalFormalLinkage, but the problem with it is that now the checker warns not only for the redundant extern usage, but for the unnecessary too - in the latter case the linkage is internal, and I think by checking the storage class we can determine whether the extern keyword has been used or not.

44–45

Could you please help me how could I achieve that? I did not find any matchers which match for macros.

lebedev.ri added inline comments.Jun 12 2019, 10:06 AM
clang-tidy/readability/RedundantExternCheck.cpp
44–45

You can simply add your own matchers, see e.g. AST_MATCHER()'s in AvoidCArraysCheck.cpp.

koldaniel marked an inline comment as done.Jun 19 2019, 6:35 AM
koldaniel added inline comments.
clang-tidy/readability/RedundantExternCheck.cpp
44–45

I see, I can update the checker based on your previous comment (move if (FD->getStorageClass() != SC_Extern) to registerMatchers()), but moving the macro check would disable the warning. We do not want to ignore the macro findings, we just don't want to apply fix-its.

koldaniel updated this revision to Diff 206190.Jun 24 2019, 4:44 AM

Hi, do you have any additional comments?

Hi, do you have any additional comments?

Could you mark "Done" the comments you've addressed?

alexfh added inline comments.Jul 17 2019, 5:56 AM
clang-tidy/readability/RedundantExternCheck.cpp
39–41

These messages alone will be quite confusing unless the user has enough context. Please expand the messages with the explanations.

49–51

I suspect there are many ways the extern substring can appear in a function definition (void my_extern(), [[some_attribute("extern")]] void f(), void /*extern*/ f(), etc.). I don't immediately see how these possible false positives are handled here. Am I missing something obvious?

Is it more robust to re-lex the range and search for the raw identifier token with the text extern?