[clang-tidy] new readability-redundant-inline

[mgehre]

Summary

This check flags redundant 'inline' specifiers.
It flags 'inline' on member functions defined inside a class definition
like
.. code-block:: c++

  struct S {
    inline int f() {
	  return 0;

}

};

and 'inline' specifiers on functions that are also declared 'constexpr'.

[Eugene.Zelenko]

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

[Eugene.Zelenko]

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

7

Please use `` for language construction highlighting. Same for other occurrences of inline and constexpr.

[mgehre]

Update for review comments

[mgehre]

I'm thinking about extending the check to the following issue and would like to hear your opinion.
In C++, the following three code snippets all have identical meaning
1:

struct S {
  int f();
};

inline int S::f() {
 return 0;
}

2:

struct S {
  inline int f();
};
int S::f() {
 return 0;
}

3:

struct S {
  inline int f();
};

inline int S::f() {
 return 0;
}

I personally think that 1) should be used, because late one could move the function definition to a source file (removing the inline) without having to touch
the class declaration. I can extend this patch to transform 2) and 3) into 1).

Alternatively, I could add an option to choose between 1), 2) or 3).
What do you think?

[sbenza]

clang-tidy/readability/RedundantInlineCheck.cpp

60

Parsing C++ is hard.
Stopping at the first { means you will have a false negative here:

template <bool> struct S{};
S<bool{}> inline foo() { return {}; }

I think we should just continue until we find the 'inline' keyword. We already know it is there from the matching.

We could also limit the search until functionDecl->getLocation(). We don't have to look until LocEnd() because we won't find it after the function name.

106

Maybe ParseTokens should just return the specific token we are looking for.

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

13

bad alignment

[mgehre]

Update for review comments; simplified lexing, added proposed test case

[mgehre]

Removed debug output

[rsmith]

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

6

The inline keyword here is not actually redundant to Clang -- we generate an LLVM inlinehint if inline is specified here and not otherwise.

[mgehre]

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

7

Personally, I never use "inline" to mean anything else than "multiple definitions can appear". I didn't know that
compilers still respected this.

Does that mean that the whole checker is useless?

[sbenza]

clang-tidy/readability/RedundantInlineCheck.cpp

69

This is still reachable.
The token might be hidden through macros, for example.
But even then we still can keep a simple API. The possible return values are a token or 'not found'.
Optional<Token> should be ok.

[mgehre]

Update for review comment:

• Return Optional<Token> when looking for 'inline'
• Add test that hides 'inline' in a macro

[mgehre]

Friendly ping

[aaron.ballman]

clang-tidy/readability/RedundantInlineCheck.cpp

24

const NamedDecl (same for const auto * below).

46

80 col limit? Should just run clang-format over the whole patch.

103–106

I think that you should always emit a PartialDiagnostic and then if Tok, add the FixItHint -- it should make it more clear what's going on.

clang-tidy/readability/RedundantInlineCheck.h

19

with inline body -> with an inline body

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

7

I don't think it means that this checker is useless, but we should definitely make sure that the checker isn't suggesting changes that have unintended impact like this.

Perhaps there's a way to have a test which generates llvm ir for the original code and compares it against llvm ir generated after automatically applying all fixits to ensure the salient data are the same? That would also make this check a bit more forward-compatible with anyone who changes the behavior of where we specify inlinehint.

77

You should add a template test, because explicit specializations are special.

template <typename T>
struct S {
    void f();
}
template <typename T>
void S<T>::f() {}  // implicitly inline

template <>
void S<int>::f() {}  // Not implicitly inline

[sbenza]

In D18914#396149, @mgehre wrote:

...
I personally think that 1) should be used, because late one could move the function definition to a source file (removing the inline) without having to touch
the class declaration. I can extend this patch to transform 2) and 3) into 1).

Alternatively, I could add an option to choose between 1), 2) or 3).
What do you think?

I agree that (1) is preferred as it makes inline an implementation detail and doesn't pollute the class, but that is a style choice.
What we could do is transform (3) to (1). That is, if you provide _both_ inlines we remove one as it is redundant. That matches with the purpose of this check.

But changing (2) to (1) is not removing anything redundant, it is a style change.

[mgehre]

Thank you for the review input! After learning about inlinehint, I don't feel its worthwhile for me to continue this check.

[chfast]

This check can be useful in other case like this:

inline constexpr const int x = 1;

where inline and const are redundant.

[Izaron]

In D18914#2399705, @chfast wrote:

This check can be useful in other case like this:

inline constexpr const int x = 1;

where inline and const are redundant.

In your case inline is not actually redundant (at least for a global variable). const (therefore constexpr) means internal linkage, so every translation unit will just get itself a unique internal copy of this variable. We can see it if we print the address of x in different translation units. The addresses will be different without inline and the same with inline.

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ContainerSizeEmptyCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolCastCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   NamedParameterCheck.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantControlFlowCheck.cpp
+  RedundantInlineCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringInitCheck.cpp
   SimplifyBooleanExprCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTooling
   )

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ContainerSizeEmptyCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolCastCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "NamedParameterCheck.h"
 #include "RedundantControlFlowCheck.h"
+#include "RedundantInlineCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
 #include "SimplifyBooleanExprCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 
 namespace clang {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolCastCheck>(
         "readability-implicit-bool-cast");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
+    CheckFactories.registerCheck<RedundantInlineCheck>(
+        "readability-redundant-inline");
     CheckFactories.registerCheck<StaticDefinitionInAnonymousNamespaceCheck>(
         "readability-static-definition-in-anonymous-namespace");
     CheckFactories.registerCheck<readability::NamedParameterCheck>(
         "readability-named-parameter");
     CheckFactories.registerCheck<RedundantControlFlowCheck>(
         "readability-redundant-control-flow");
     CheckFactories.registerCheck<RedundantSmartptrGetCheck>(
         "readability-redundant-smartptr-get");

clang-tidy/readability/RedundantInlineCheck.h

+//===--- RedundantInlineCheck.h - clang-tidy---------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_INLINE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANT_INLINE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Flags redundant 'inline' when used on a method with inline body or on a constexpr function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-inline.html
+class RedundantInlineCheck : public ClangTidyCheck {
+public:
+  RedundantInlineCheck(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_INLINE_H

clang-tidy/readability/RedundantInlineCheck.cpp

+//===--- RedundantInlineCheck.cpp - clang-tidy-----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RedundantInlineCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+AST_MATCHER(CXXMethodDecl, hasInlineBody) { return Node.hasInlineBody(); }
+
+AST_MATCHER(FriendDecl, isInlineAndHasBody) {
+  NamedDecl *D = Node.getFriendDecl();
+  if (!D)
+    return false;
+  auto *F = dyn_cast<FunctionDecl>(D);
+  if (!F)
+    return false;
+  return F->isThisDeclarationADefinition() && F->isInlineSpecified();
+}
+
+void RedundantInlineCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  Finder->addMatcher(functionDecl(isInline(), isConstexpr()).bind("func"),
+                     this);
+  Finder->addMatcher(
+      cxxMethodDecl(isInline(), unless(isConstexpr()), hasInlineBody())
+          .bind("method"),
+      this);
+  Finder->addMatcher(friendDecl(isInlineAndHasBody()).bind("friend"), this);
+}
+
+// Re-lex the tokens to get precise location of 'inline'
+static Optional<Token> InlineTok(CharSourceRange Range, const MatchFinder::MatchResult &Result) {
+  const SourceManager &Sources = *Result.SourceManager;
+  std::pair<FileID, unsigned> LocInfo =
+      Sources.getDecomposedLoc(Range.getBegin());
+  StringRef File = Sources.getBufferData(LocInfo.first);
+  const char *TokenBegin = File.data() + LocInfo.second;
+  Lexer RawLexer(Sources.getLocForStartOfFile(LocInfo.first),
+                 Result.Context->getLangOpts(), File.begin(), TokenBegin,
+                 File.end());
+  Token Tok;
+  while (!RawLexer.LexFromRawLexer(Tok)) {
+    if (Tok.is(tok::raw_identifier)) {
+      IdentifierInfo &Info = Result.Context->Idents.get(StringRef(
+        Sources.getCharacterData(Tok.getLocation()), Tok.getLength()));
+      Tok.setIdentifierInfo(&Info);
+      Tok.setKind(Info.getTokenID());
+    }
+    if (Tok.is(tok::kw_inline))
+      return Tok;
+    if (Sources.isBeforeInTranslationUnit(Range.getEnd(), Tok.getLocation()))
+      break;
+  }
+  return None;
+}
+
+void RedundantInlineCheck::check(const MatchFinder::MatchResult &Result) {
+  const char *Msg;
+  SourceRange Range;
+  SourceLocation Loc;
+  if (const auto *Matched = Result.Nodes.getNodeAs<CXXMethodDecl>("method")) {
+    Msg = "'inline' is redundant because method body is defined inside class";
+    Range = SourceRange(Matched->getSourceRange().getBegin(), Matched->getLocation());
+    Loc = Matched->getLocation();
+  } else if (const auto *Matched =
+                 Result.Nodes.getNodeAs<FunctionDecl>("func")) {
+    Msg = "'inline' is redundant because 'constexpr' implies 'inline'";
+    Range = SourceRange(Matched->getSourceRange().getBegin(), Matched->getLocation());
+    Loc = Matched->getLocation();
+  } else if (const auto *Matched =
+                 Result.Nodes.getNodeAs<FriendDecl>("friend")) {
+    Msg = "'inline' is redundant because function body is defined inside class";
+    Range = SourceRange(Matched->getSourceRange().getBegin(), Matched->getLocation());
+    Loc = Matched->getLocation();
+  } else {
+    return;
+  }
+
+  CharSourceRange FileRange = Lexer::makeFileCharRange(
+      CharSourceRange::getTokenRange(Range), *Result.SourceManager,
+      Result.Context->getLangOpts());
+
+  if (!FileRange.isValid())
+    return;
+
+  // FIXME: Instead of re-lexing, properly store the location of 'inline' in
+  // each FunctionDecl.
+  Optional<Token> Tok = InlineTok(FileRange, Result);
+  if (Tok)
+    diag(Loc, Msg) << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(Tok->getLocation(), Tok->getLocation()));
+  else
+    diag(Loc, Msg);
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New `readability-avoid-const-params-in-decls
   Warns about top-level const parameters in function declarations.
 
 - New `readability-redundant-control-flow
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-control-flow.html>`_ check
 
   Looks for procedures (functions returning no value) with ``return`` statements
   at the end of the function.  Such `return` statements are redundant.
 
+- New `readability-redundant-inline
+  <http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-inline.html>`_ check
+
+  Looks for redundant ``inline`` specifiers which are implied by defining a body within a class definition
+  or by ``constexpr``.
+
 - New `readability-redundant-string-init
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-init.html>`_ check
 
   Finds unnecessary string initializations.
 
 - New `readability-static-definition-in-anonymous-namespace
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.html>`_ check
 

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-container-size-empty
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-named-parameter
    readability-redundant-control-flow
+   readability-redundant-inline
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release

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

+.. title:: clang-tidy - readability-redundant-inline
+
+readability-redundant-inline
+============================
+
+This check flags redundant ``inline`` specifiers.
+It flags ``inline`` on member functions defined inside a class definition like
+.. code-block:: c++
+
+  struct S {
+    inline int f() {
+      return 0;
+    }
+  };
+
+and ``inline`` specifiers on functions that are also declared ``constexpr``.

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

+// RUN: %check_clang_tidy %s readability-redundant-inline %t
+
+struct S {
+  inline int f1() {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'inline' is redundant because method body is defined inside class [readability-redundant-inline]
+// CHECK-FIXES: {{^}}  int f1()
+    return 0;
+  }
+  inline int f2(); // OK
+
+  inline constexpr int f3() {
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'inline' is redundant because 'constexpr' implies 'inline' [readability-redundant-inline]
+// CHECK-FIXES: {{^}}  constexpr int f3()
+    return 0;
+  }
+  static inline constexpr int f4() {
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 'inline' is redundant because 'constexpr' implies 'inline'
+// CHECK-FIXES: {{^}}  static constexpr int f4()
+    return 0;
+  }
+
+  static inline int f5();
+
+  static inline int f6() {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'inline' is redundant because method body is defined inside class
+// CHECK-FIXES: {{^}}  static int f6()
+    return 0;
+  }
+
+  inline friend int f7() {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'inline' is redundant because function body is defined inside class [readability-redundant-inline]
+// CHECK-FIXES: {{^}}  friend int f7()
+      return 0;
+  }
+
+  inline friend int f8(); // OK
+
+  template<bool> struct T{};
+  struct T<bool{}> inline f9() { return {}; }
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 'inline' is redundant because method body is defined inside class
+// CHECK-FIXES: {{^}}  struct T<bool{}> f9() { return {}; }
+
+#define INLINE inline
+  INLINE void f10() {
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'inline' is redundant because method body is defined inside class
+  }
+};
+
+class S2 {
+  inline int f1() {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'inline' is redundant because method body is defined inside class
+// CHECK-FIXES: {{^}}  int f1()
+    return 0;
+  }
+};
+
+inline int S::f2() { // OK
+  return 0;
+}
+
+inline int S::f5() { // OK
+  return 0;
+}
+
+inline int f3() { // OK
+  return 0;
+}
+
+inline constexpr int f4() {
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 'inline' is redundant because 'constexpr' implies 'inline'
+// CHECK-FIXES: {{^}}constexpr int f4()
+  return 1;
+}
+
+constexpr int f5() { // OK
+  return 1;
+}