Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy] Added new check to detect redundant inline keyword #73069

Merged
merged 2 commits into from
Jan 20, 2024

Conversation

felix642
Copy link
Contributor

This checks find usages of the inline keywork where it is already implicitly defined by the compiler and suggests it's removal.

Fixes #72397

@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tidy labels Nov 22, 2023
@felix642 felix642 changed the title [clang-tidy] Added check to detect redundant inline keyword [clang-tidy] Added new check to detect redundant inline keyword Nov 22, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 22, 2023

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang

Author: Félix-Antoine Constantin (felix642)

Changes

This checks find usages of the inline keywork where it is already implicitly defined by the compiler and suggests it's removal.

Fixes #72397


Full diff: https://github.com/llvm/llvm-project/pull/73069.diff

9 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/readability/CMakeLists.txt (+1)
  • (modified) clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp (+3)
  • (added) clang-tools-extra/clang-tidy/readability/RedundantInlineSpecifierCheck.cpp (+94)
  • (added) clang-tools-extra/clang-tidy/readability/RedundantInlineSpecifierCheck.h (+36)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
  • (added) clang-tools-extra/docs/clang-tidy/checks/readability/redundant-inline-specifier.rst (+34)
  • (added) clang-tools-extra/test/clang-tidy/checkers/readability/redundant-inline-specifier.cpp (+110)
  • (modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+1-1)
diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index 5452c2d48a46173..811310db8c721a6 100644
--- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -20,6 +20,7 @@ add_clang_library(clangTidyReadabilityModule
   IdentifierLengthCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
+  RedundantInlineSpecifierCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
   MagicNumbersCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index b8e6e6414320600..3ce7bfecaecba64 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -39,6 +39,7 @@
 #include "RedundantControlFlowCheck.h"
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
+#include "RedundantInlineSpecifierCheck.h"
 #include "RedundantMemberInitCheck.h"
 #include "RedundantPreprocessorCheck.h"
 #include "RedundantSmartptrGetCheck.h"
@@ -93,6 +94,8 @@ class ReadabilityModule : public ClangTidyModule {
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolConversionCheck>(
         "readability-implicit-bool-conversion");
+    CheckFactories.registerCheck<RedundantInlineSpecifierCheck>(
+        "readability-redundant-inline-specifier");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
         "readability-inconsistent-declaration-parameter-name");
     CheckFactories.registerCheck<IsolateDeclarationCheck>(
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantInlineSpecifierCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantInlineSpecifierCheck.cpp
new file mode 100644
index 000000000000000..7b67a7419c708b7
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/RedundantInlineSpecifierCheck.cpp
@@ -0,0 +1,94 @@
+//===--- RedundantInlineSpecifierCheck.cpp - clang-tidy----------------------===//
+//
+// 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 "RedundantInlineSpecifierCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+
+#include "../utils/LexerUtils.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::readability {
+
+static std::optional<SourceLocation>
+getInlineTokenLocation(SourceRange RangeLocation,
+                       const SourceManager &Sources,
+                       const LangOptions &LangOpts) {
+  SourceLocation CurrentLocation = RangeLocation.getBegin();
+  Token CurrentToken;
+  while (!Lexer::getRawToken(CurrentLocation, CurrentToken, Sources, LangOpts,
+                             true) && CurrentLocation < RangeLocation.getEnd() && CurrentToken.isNot(tok::eof)) {
+    if (CurrentToken.is(tok::raw_identifier)) {
+      if (CurrentToken.getRawIdentifier() == "inline") {
+        return CurrentToken.getLocation();
+      }
+    }
+    CurrentLocation = CurrentToken.getEndLoc();
+  }
+  return std::nullopt;
+}
+
+void RedundantInlineSpecifierCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      functionDecl(unless(isExpansionInSystemHeader()), unless(isImplicit()),
+                   unless(hasAncestor(lambdaExpr())), isInline(),
+                   anyOf(isConstexpr(), isDeleted(),
+                         allOf(isDefinition(), hasAncestor(recordDecl()),
+                               unless(hasAncestor(cxxConstructorDecl())))))
+          .bind("fun_decl"),
+      this);
+
+  Finder->addMatcher(
+      varDecl(isInline(), unless(isImplicit()),
+              anyOf(allOf(isConstexpr(), unless(isStaticStorageClass())),
+                    hasAncestor(recordDecl())))
+          .bind("var_decl"),
+      this);
+
+  Finder->addMatcher(
+    functionTemplateDecl(has(functionDecl(isInline()))).bind("templ_decl"), this
+  );
+}
+
+template <typename T>
+void RedundantInlineSpecifierCheck::handleMatchedDecl(const T *MatchedDecl, const SourceManager &Sources,
+                       const MatchFinder::MatchResult &Result,
+                       const char *Message) {
+  if (auto Loc = getInlineTokenLocation(MatchedDecl->getSourceRange(),
+                                        Sources, Result.Context->getLangOpts()))
+    diag(*Loc, Message) << MatchedDecl << FixItHint::CreateRemoval(*Loc);
+}
+
+void RedundantInlineSpecifierCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const SourceManager &Sources = *Result.SourceManager;
+
+  if (const auto *MatchedDecl =
+          Result.Nodes.getNodeAs<FunctionDecl>("fun_decl")) {
+    handleMatchedDecl(MatchedDecl, Sources, Result,
+                      "Function %0 has inline specifier but is implicitly inlined");
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<VarDecl>("var_decl")) {
+    handleMatchedDecl(MatchedDecl, Sources, Result,
+                      "Variable %0 has inline specifier but is implicitly inlined");
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<FunctionTemplateDecl>("templ_decl")) {
+    handleMatchedDecl(MatchedDecl, Sources, Result,
+                      "Function %0 has inline specifier but is implicitly inlined");
+  }
+}
+
+} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantInlineSpecifierCheck.h b/clang-tools-extra/clang-tidy/readability/RedundantInlineSpecifierCheck.h
new file mode 100644
index 000000000000000..342e56044a42b1d
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/RedundantInlineSpecifierCheck.h
@@ -0,0 +1,36 @@
+//===--- RedundantInlineSpecifierCheck.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_REDUNDANTINLINESPECIFIERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTINLINESPECIFIERCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// Finds redundant `inline` specifiers in function and variable declarations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/readability-redundant-inline-specifier.html
+class RedundantInlineSpecifierCheck : public ClangTidyCheck {
+public:
+  RedundantInlineSpecifierCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+template <typename T>
+void handleMatchedDecl(const T *MatchedDecl, const SourceManager &Sources,
+                       const ast_matchers::MatchFinder::MatchResult &Result,
+                       const char *Message);
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTINLINESPECIFIERCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f49c412118e7d98..2b09f4fd8a0f65c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -192,6 +192,11 @@ New checks
   Recommends the smallest possible underlying type for an ``enum`` or ``enum``
   class based on the range of its enumerators.
 
+- New :doc:`readability-redundant-inline-specifier
+  <clang-tidy/checks/readability/readability-redundant-inline-specifier>` check.
+
+  Detects redundant ``inline`` specifiers on function and variable declarations.
+
 - New :doc:`readability-reference-to-constructed-temporary
   <clang-tidy/checks/readability/reference-to-constructed-temporary>` check.
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 6f987ba1672e3f2..b95d8ed64fa180d 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -348,6 +348,7 @@ Clang-Tidy Checks
    :doc:`readability-identifier-length <readability/identifier-length>`,
    :doc:`readability-identifier-naming <readability/identifier-naming>`, "Yes"
    :doc:`readability-implicit-bool-conversion <readability/implicit-bool-conversion>`, "Yes"
+   :doc:`readability-redundant-inline-specifier <readability/readability-redundant-inline-specifier>`, "Yes"
    :doc:`readability-inconsistent-declaration-parameter-name <readability/inconsistent-declaration-parameter-name>`, "Yes"
    :doc:`readability-isolate-declaration <readability/isolate-declaration>`, "Yes"
    :doc:`readability-magic-numbers <readability/magic-numbers>`,
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-inline-specifier.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-inline-specifier.rst
new file mode 100644
index 000000000000000..a066f137e0a71c2
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/redundant-inline-specifier.rst
@@ -0,0 +1,34 @@
+.. title:: clang-tidy - readability-redundant-inline-specifier
+
+readability-redundant-inline-specifier
+=================
+
+Checks for instances of the `inline` keyword in code where it is redundant
+and recommends its removal.
+
+Examples:
+
+.. code-block:: c++
+
+   constexpr inline void f() {}
+
+In the example abvove the keyword `inline` is redundant since constexpr
+functions are implicitly inlined
+
+.. code-block:: c++
+   class MyClass {
+       inline void myMethod() {}
+   };
+
+In the example above the keyword `inline` is redundant since member functions
+defined entirely inside a class/struct/union definition are implicitly inlined.
+
+The token `inline` is considered redundant in the following cases:
+
+- When it is used in a function definition that is constexpr.
+- When it is used in a member function definition that is defined entirely
+  inside a class/struct/union definition.
+- When it is used on a deleted function. 
+- When it is used on a template declaration.
+- When it is used on a member variable that is constexpr and static.
+
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-inline-specifier.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-inline-specifier.cpp
new file mode 100644
index 000000000000000..b9b143bee46f2a9
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-inline-specifier.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s readability-redundant-inline-specifier %t
+
+template <typename T> inline T f()
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: Function 'f' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+// CHECK-FIXES: template <typename T> T f()
+{
+    return T{};
+}
+
+template <> inline double f<double>() = delete;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Function 'f<double>' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+// CHECK-FIXES: template <> double f<double>() = delete;
+
+inline int g(float a)
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:1: warning: Function 'g' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+{
+    return static_cast<int>(a - 5.F);
+}
+
+inline int g(double) = delete;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: Function 'g' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+// CHECK-FIXES: int g(double) = delete;
+
+class C
+{
+  public:
+    inline C& operator=(const C&) = delete;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Function 'operator=' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    // CHECK-FIXES: C& operator=(const C&) = delete;
+
+    constexpr inline C& operator=(int a);
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: Function 'operator=' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    // CHECK-FIXES: constexpr C& operator=(int a);
+
+    inline C() {}
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Function 'C' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    // CHECK-FIXES: C() {}
+
+    constexpr inline C(int);
+    // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: Function 'C' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    // CHECK-FIXES: constexpr C(int);
+
+    inline int Get42() const { return 42; }
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Function 'Get42' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    // CHECK-FIXES: int Get42() const { return 42; }
+
+    static inline constexpr int C_STATIC = 42;
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: Variable 'C_STATIC' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    // CHECK-FIXES: static constexpr int C_STATIC = 42;
+
+    static constexpr int C_STATIC_2 = 42;
+    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: Variable 'C_STATIC_2' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+};
+
+constexpr inline int Get42() { return 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: Function 'Get42' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+// CHECK-FIXES: constexpr int Get42() { return 42; }
+
+
+static constexpr inline int NAMESPACE_STATIC = 42;
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:18: warning: Variable 'NAMESPACE_STATIC' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+
+inline static int fn0(int i)
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]:1: warning: Function 'fn0' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+{
+    return i - 1;
+}
+
+static constexpr inline int fn1(int i)
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: Function 'fn1' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+// CHECK-FIXES: static constexpr int fn1(int i)
+{
+    return i - 1;
+}
+
+namespace
+{
+    inline int fn2(int i)
+    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: Function 'fn2' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    {
+        return i - 1;
+    }
+
+    inline constexpr int fn3(int i)
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Function 'fn3' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    // CHECK-FIXES: constexpr int fn3(int i)
+    {
+        return i - 1;
+    }
+}
+
+namespace ns
+{
+    inline int fn4(int i)
+    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:5: warning: Function 'fn4' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    {
+        return i - 1;
+    }
+
+    inline constexpr int fn5(int i)
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Function 'fn5' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+    // CHECK-FIXES: constexpr int fn5(int i)
+    {
+        return i - 1;
+    }
+}
+
+auto fn6 = [](){};
+//CHECK-MESSAGES-NOT: :[[@LINE-1]]:1: warning: Function 'operator()' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]
+
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 82a26356c58f556..06e784649a975bf 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -7889,7 +7889,7 @@ AST_POLYMORPHIC_MATCHER(isInline, AST_POLYMORPHIC_SUPPORTED_TYPES(NamespaceDecl,
   if (const auto *NSD = dyn_cast<NamespaceDecl>(&Node))
     return NSD->isInline();
   if (const auto *VD = dyn_cast<VarDecl>(&Node))
-    return VD->isInline();
+    return VD->isInlineSpecified();
   llvm_unreachable("Not a valid polymorphic type");
 }
 

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not bad, just few nits.

@philnik777
Copy link
Contributor

@PiotrZSL could you use the "Start review" button to avoid spamming people with dozens of E-Mails for a single review?

@PiotrZSL
Copy link
Member

@PiotrZSL could you use the "Start review" button to avoid spamming people with dozens of E-Mails for a single review?

@philnik777 If you prefer not to receive notifications, simply unsubscribe. Specially if you consider such notifications a "spam".

@philnik777
Copy link
Contributor

@PiotrZSL could you use the "Start review" button to avoid spamming people with dozens of E-Mails for a single review?

@philnik777 If you prefer not to receive notifications, simply unsubscribe. Specially if you consider such notifications a "spam".

It's not that I don't want to receive notifications. I want to receive one E-Mail for one review. I'm also not a fan of receiving dozens of mails for a review on one of my patches, but I still want the notification. A single mail for such a thing isn't spam. Getting eight is.
I realize that the GitHub interface makes it really easy to do this, but there is a way around it (other than not using GitHub).

Copy link
Contributor

@bjosv bjosv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some nit/low-prio comments.

Copy link

github-actions bot commented Nov 23, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@EugeneZelenko EugeneZelenko removed the clang Clang issues not falling into any other category label Nov 25, 2023
Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine (except comments), main concern is if this isn't to strict, simply if someone remove some of those inline keywords, won't it decrease performance of code.

Please add verification, to ignore any functions with attributes, mainly always_inline, never_inline but to simplify all could be ignored.

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I think this could be merged.
I was thinking, maybe with current scope of check name readability-redundant-inline could be sufficient.

@felix642
Copy link
Contributor Author

ping @EugeneZelenko

This checks find usages of the inline keywork where it is
already implicitly defined by the compiler and suggests it's removal.

Fixes llvm#72397
@felix642
Copy link
Contributor Author

I have rebased my branch with main.
@PiotrZSL if you could commit those changes for me I would greatly appreciate it.

@PiotrZSL PiotrZSL merged commit 7a8f5d9 into llvm:main Jan 20, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] clang-tidy check for implicit inline
7 participants