[clang-tidy] redundant 'extern' keyword check

[koldaniel]

Summary

finds and removes redundant 'extern' keywords

[mgehre]

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]

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]

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

[Eugene.Zelenko]

In D33841#771944, @Prazek wrote:

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

This will duplicate modernize-redundant-void-arg.

[Prazek]

extern on function definition is also redundant, right?

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

[alexfh]

docs/clang-tidy/checks/readability-redundant-keyword.rst

8 ↗

(On Diff #101234)

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

[aaron.ballman]

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]

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]

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]

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]

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

[koldaniel]

Rebased.

[Eugene.Zelenko]

Please mention new check in Release Notes.

clang-tidy/readability/RedundantExternCheck.cpp

71

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]

clang-tidy/readability/RedundantExternCheck.cpp

46

current convention would be that this should be Offset

[aaron.ballman]

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

5

The underlining here is too long.

31

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]

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]

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]

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]

Typedef and macro maches fixed, new test cases added.

[aaron.ballman]

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]

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]

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]

Updating warnings.

[lebedev.ri]

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]

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]

clang-tidy/readability/RedundantExternCheck.cpp

44–45

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

[koldaniel]

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]

Hi, do you have any additional comments?

[alexfh]

In D33841#1589212, @koldaniel wrote:

Hi, do you have any additional comments?

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

[alexfh]

clang-tidy/readability/RedundantExternCheck.cpp

38–40

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

48–50

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?

clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   NonConstParameterCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
   RedundantDeclarationCheck.cpp
+  RedundantExternCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp
   RedundantPreprocessorCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
   SimplifySubscriptExprCheck.cpp

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "MagicNumbersCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
 #include "NonConstParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
+#include "RedundantExternCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantPreprocessorCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "SimplifySubscriptExprCheck.h"
 #include "StaticAccessedThroughInstanceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<IsolateDeclarationCheck>(
         "readability-isolate-declaration");
     CheckFactories.registerCheck<MagicNumbersCheck>(
         "readability-magic-numbers");
     CheckFactories.registerCheck<MisleadingIndentationCheck>(
         "readability-misleading-indentation");
     CheckFactories.registerCheck<MisplacedArrayIndexCheck>(
         "readability-misplaced-array-index");
+    CheckFactories.registerCheck<RedundantExternCheck>(
+        "readability-redundant-extern");
     CheckFactories.registerCheck<RedundantFunctionPtrDereferenceCheck>(
         "readability-redundant-function-ptr-dereference");
     CheckFactories.registerCheck<RedundantMemberInitCheck>(
         "readability-redundant-member-init");
     CheckFactories.registerCheck<RedundantPreprocessorCheck>(
         "readability-redundant-preprocessor");
     CheckFactories.registerCheck<SimplifySubscriptExprCheck>(
         "readability-simplify-subscript-expr");

clang-tidy/readability/RedundantExternCheck.h

+//===--- RedundantExternCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_EXTERN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_EXTERN_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Finds redundant 'extern' keywords
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-extern.html
+class RedundantExternCheck : public ClangTidyCheck {
+public:
+  RedundantExternCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_EXTERN_H

clang-tidy/readability/RedundantExternCheck.cpp

+//===--- RedundantExternCheck.cpp - clang-tidy ------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantExternCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+AST_MATCHER(clang::FunctionDecl, hasExternStorageClass) {
+  return Node.getStorageClass() == clang::SC_Extern;
+}
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void RedundantExternCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(functionDecl(hasExternStorageClass(), unless(isExternC()))
+                         .bind("redundant_extern"),
+                     this);
+}
+
+void RedundantExternCheck::check(const MatchFinder::MatchResult &Result) {
+  auto *FD = Result.Nodes.getNodeAs<FunctionDecl>("redundant_extern");
+  SourceLocation BeginLoc = FD->getBeginLoc();
+  SourceLocation EndLoc = FD->getEndLoc();
+
+  StringRef Message;
+
+  if (FD->getLinkageInternal() == InternalLinkage) {
+    Message = "'extern' keyword has no effect";
+  } else {
+    Message = "redundant 'extern' keyword";
+  }
+
+  auto Diag = diag(FD->getBeginLoc(), Message);
+
+  if (FD->getBeginLoc().isMacroID())
+    return;
+
+  StringRef Text =
+      Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
+                           *Result.SourceManager, getLangOpts());
+
+  llvm::StringLiteral Extern = llvm::StringLiteral("extern");
+  int Offset = Text.find(Extern);
+
+  SourceRange ExternRange(BeginLoc.getLocWithOffset(Offset),
+                          BeginLoc.getLocWithOffset(Offset + Extern.size()));
+
+  StringRef ExternText =
+      Lexer::getSourceText(CharSourceRange::getTokenRange(ExternRange),
+                           *Result.SourceManager, getLangOpts());
+
+  if (!ExternText.equals(Extern)) // Final check: typedefs, etc.
+    return;
+
+  Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(ExternRange));
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 
 - New :doc:`openmp-use-default-none
   <clang-tidy/checks/openmp-use-default-none>` check.
 
   Finds OpenMP directives that are allowed to contain a ``default`` clause,
   but either don't specify it or the clause is specified but with the kind
   other than ``none``, and suggests to use the ``default(none)`` clause.
 
+- New :doc:`readability-redundant-extern
+  <clang-tidy/checks/readability-redundant-extern>` check.
+
+  Removes the redundant ``extern`` keywords.
+
 Improvements to clang-include-fixer
 -----------------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...
 
 Improvements to pp-trace
 ------------------------
 
 - Added a new option `-callbacks` to filter preprocessor callbacks. It replaces
   the `-ignore` option.

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

+.. title:: clang-tidy - readability-redundant-extern
+
+readability-redundant-extern
+============================
+
+Removes the redundant or unnecessary ``extern`` keywords from code.
+
+``extern`` is redundant in function declarations, and has no effect in
+namespaces
+
+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
+
+.. code-block:: c++
+
+  extern void h();
+
+  namespace {
+    extern void namespace_1();  
+  }
+
+  #define FOO_EXTERN extern
+  FOO_EXTERN void foo_macro_1();
+
+  #define FOO_EXTERN extern
+  typedef int extern_int;
+  extern_int FOO_EXTERN typedef_extern_foo();
+

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

+// RUN: %check_clang_tidy %s readability-redundant-extern %t
+
+extern int f();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: int f();
+
+int extern f() { return 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: int f() { return 0; }
+
+extern "C" int g();
+// CHECK-FIXES: extern "C" int g();
+
+extern "C" int g() { return 0; }
+// CHECK-FIXES: extern "C" int g() { return 0; }
+
+extern "C++" int h();
+// CHECK-FIXES: extern "C++" int h();
+
+extern "C++" int h() { return 0; }
+// CHECK-FIXES: extern "C++" int h() { return 0; }
+
+inline extern void foo_inline();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: inline void foo_inline();
+
+#define FOO_EXTERN extern
+FOO_EXTERN void foo_macro_1();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: FOO_EXTERN void foo_macro_1();
+
+#define FOO_INLINE inline
+FOO_INLINE extern void foo_macro_2();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: FOO_INLINE extern void foo_macro_2();
+
+#define FOO_EXTERN_INLINE inline extern
+FOO_EXTERN_INLINE void foo_macro_3();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: FOO_EXTERN_INLINE void foo_macro_3();
+
+void file_scope();
+// CHECK-FIXES: void file_scope();
+
+void another_file_scope(int _extern);
+// CHECK-FIXES: void another_file_scope(int _extern);
+
+namespace {
+extern void namespace_1();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: 'extern' keyword has no effect [readability-redundant-extern]
+// CHECK-FIXES: void namespace_1();
+}
+
+namespace a {
+namespace {
+namespace b {
+extern void namespace_2();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: 'extern' keyword has no effect [readability-redundant-extern]
+// CHECK-FIXES: void namespace_2();
+}
+}
+}
+
+namespace {
+extern "C" void namespace_3();
+// CHECK-FIXES: extern "C" void namespace_3();
+}
+
+#define FOO_EXTERN extern
+typedef int extern_int;
+
+extern_int FOO_EXTERN typedef_extern_foo();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: extern_int FOO_EXTERN typedef_extern_foo();