[clang-tidy] Add new check llvm-prefer-static-over-anonymous-namespace

Author: vbvictor

clang-tools-extra/clang-tidy/llvm/CMakeLists.txt

@@ -9,6 +9,7 @@ add_clang_library(clangTidyLLVMModule STATIC
   LLVMTidyModule.cpp
   PreferIsaOrDynCastInConditionalsCheck.cpp
   PreferRegisterOverUnsignedCheck.cpp
+  PreferStaticOverAnonymousNamespaceCheck.cpp
   TwineLocalCheck.cpp
 
   LINK_LIBS

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

@@ -16,6 +16,7 @@
 #include "IncludeOrderCheck.h"
 #include "PreferIsaOrDynCastInConditionalsCheck.h"
 #include "PreferRegisterOverUnsignedCheck.h"
+#include "PreferStaticOverAnonymousNamespaceCheck.h"
 #include "TwineLocalCheck.h"
 
 namespace clang::tidy {
@@ -34,6 +35,8 @@ class LLVMModule : public ClangTidyModule {
         "llvm-prefer-isa-or-dyn-cast-in-conditionals");
     CheckFactories.registerCheck<PreferRegisterOverUnsignedCheck>(
         "llvm-prefer-register-over-unsigned");
+    CheckFactories.registerCheck<PreferStaticOverAnonymousNamespaceCheck>(
+        "llvm-prefer-static-over-anonymous-namespace");
     CheckFactories.registerCheck<readability::QualifiedAutoCheck>(
         "llvm-qualified-auto");
     CheckFactories.registerCheck<TwineLocalCheck>("llvm-twine-local");

clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.cpp

@@ -0,0 +1,96 @@
+//===--- PreferStaticOverAnonymousNamespaceCheck.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 "PreferStaticOverAnonymousNamespaceCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::llvm_check {
+
+namespace {
+
+AST_MATCHER(NamedDecl, isInMacro) {
+  return Node.getBeginLoc().isMacroID() || Node.getEndLoc().isMacroID();
+}
+
+AST_MATCHER(VarDecl, isLocalVariable) { return Node.isLocalVarDecl(); }
+
+} // namespace
+
+PreferStaticOverAnonymousNamespaceCheck::
+    PreferStaticOverAnonymousNamespaceCheck(StringRef Name,
+                                            ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      AllowVariableDeclarations(Options.get("AllowVariableDeclarations", true)),
+      AllowMemberFunctionsInClass(
+          Options.get("AllowMemberFunctionsInClass", true)) {}
+
+void PreferStaticOverAnonymousNamespaceCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AllowVariableDeclarations", AllowVariableDeclarations);
+  Options.store(Opts, "AllowMemberFunctionsInClass",
+                AllowMemberFunctionsInClass);
+}
+
+void PreferStaticOverAnonymousNamespaceCheck::registerMatchers(
+    MatchFinder *Finder) {
+  const auto IsDefinitionInAnonymousNamespace =
+      allOf(unless(isExpansionInSystemHeader()), isInAnonymousNamespace(),
+            unless(isInMacro()), isDefinition());
+
+  if (AllowMemberFunctionsInClass) {
+    Finder->addMatcher(functionDecl(IsDefinitionInAnonymousNamespace,
+                                    unless(hasParent(cxxRecordDecl())))
+                           .bind("function"),
+                       this);
+  } else
+    Finder->addMatcher(
+        functionDecl(IsDefinitionInAnonymousNamespace).bind("function"), this);
+
+  if (!AllowVariableDeclarations)
+    Finder->addMatcher(varDecl(IsDefinitionInAnonymousNamespace,
+                               unless(isLocalVariable()), unless(parmVarDecl()))
+                           .bind("var"),
+                       this);
+}
+
+void PreferStaticOverAnonymousNamespaceCheck::check(
+    const MatchFinder::MatchResult &Result) {
+
+  if (const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("function")) {
+    if (Func->isCXXClassMember())
+      diag(Func->getLocation(),
+           "place definition of method %0 outside of an anonymous namespace")
+          << Func;
+    else if (Func->isStatic())
+      diag(Func->getLocation(),
+           "place static function %0 outside of an anonymous namespace")
+          << Func;
+    else
+      diag(Func->getLocation(),
+           "function %0 is declared in an anonymous namespace; "
+           "prefer using 'static' for restricting visibility")
+          << Func;
+    return;
+  }
+
+  if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var")) {
+    if (Var->getStorageClass() == SC_Static)
+      diag(Var->getLocation(),
+           "place static variable %0 outside of an anonymous namespace")
+          << Var;
+    else
+      diag(Var->getLocation(),
+           "variable %0 is declared in an anonymous namespace; "
+           "prefer using 'static' for restricting visibility")
+          << Var;
+  }
+}
+
+} // namespace clang::tidy::llvm_check

clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.h

@@ -0,0 +1,42 @@
+//===--- PreferStaticOverAnonymousNamespaceCheck.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_LLVM_PREFERSTATICOVERANONYMOUSNAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_PREFERSTATICOVERANONYMOUSNAMESPACECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::llvm_check {
+
+/// Finds function and variable declarations inside anonymous namespace and
+/// suggests replacing them with ``static`` declarations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvm/prefer-static-over-anonymous-namespace.html
+class PreferStaticOverAnonymousNamespaceCheck : public ClangTidyCheck {
+public:
+  PreferStaticOverAnonymousNamespaceCheck(StringRef Name,
+                                          ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+
+private:
+  const bool AllowVariableDeclarations;
+  const bool AllowMemberFunctionsInClass;
+};
+
+} // namespace clang::tidy::llvm_check
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_PREFERSTATICOVERANONYMOUSNAMESPACECHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -142,6 +142,12 @@ New checks
   Finds unscoped (non-class) ``enum`` declarations and suggests using
   ``enum class`` instead.
 
+- New :doc:`llvm-prefer-static-over-anonymous-namespace
+  <clang-tidy/checks/llvm/prefer-static-over-anonymous-namespace>` check.
+
+  Finds function and variable declarations inside anonymous namespace and
+  suggests replacing them with ``static`` declarations.
+
 - New :doc:`modernize-use-scoped-lock
   <clang-tidy/checks/modernize/use-scoped-lock>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -250,6 +250,7 @@ Clang-Tidy Checks
    :doc:`llvm-namespace-comment <llvm/namespace-comment>`,
    :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm/prefer-isa-or-dyn-cast-in-conditionals>`, "Yes"
    :doc:`llvm-prefer-register-over-unsigned <llvm/prefer-register-over-unsigned>`, "Yes"
+   :doc:`llvm-prefer-static-over-anonymous-namespace <llvm/prefer-static-over-anonymous-namespace>`,
    :doc:`llvm-twine-local <llvm/twine-local>`, "Yes"
    :doc:`llvmlibc-callee-namespace <llvmlibc/callee-namespace>`,
    :doc:`llvmlibc-implementation-in-namespace <llvmlibc/implementation-in-namespace>`,

clang-tools-extra/docs/clang-tidy/checks/llvm/prefer-static-over-anonymous-namespace.rst

@@ -0,0 +1,73 @@
+.. title:: clang-tidy - llvm-prefer-static-over-anonymous-namespace
+
+llvm-prefer-static-over-anonymous-namespace
+===========================================
+
+Finds function and variable declarations inside anonymous namespace and
+suggests replacing them with ``static`` declarations.
+
+The `LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html#restrict-visibility>`_
+recommend keeping anonymous namespaces as small as possible and only use them
+for class declarations. For functions and variables, ``static`` specifier
+should be preferred for restricting visibility.
+
+For example non-compliant code:
+
+.. code-block:: c++
+
+  namespace {
+
+  class StringSort {
+  public:
+    StringSort(...)
+    bool operator<(const char *RHS) const;
+  };
+
+  // warning: place method definition outside of an anonymous namespace
+  bool StringSort::operator<(const char *RHS) const {}
+
+  // warning: prefer using 'static' for restricting visibility
+  void runHelper() {}
+
+  // warning: prefer using 'static' for restricting visibility
+  int myVariable = 42;
+
+  }
+
+Should become:
+
+.. code-block:: c++
+  
+  // Small anonymous namespace for class declaration
+  namespace {
+
+  class StringSort {
+  public:
+    StringSort(...)
+    bool operator<(const char *RHS) const;
+  };
+
+  }
+  
+  // placed method definition outside of the anonymous namespace
+  bool StringSort::operator<(const char *RHS) const {}
+
+  // used 'static' instead of an anonymous namespace
+  static void runHelper() {}
+
+  // used 'static' instead of an anonymous namespace
+  static int myVariable = 42;
+
+
+Options
+-------
+
+.. option:: AllowVariableDeclarations
+
+  When `true`, allow variable declarations to be in anonymous namespace.
+  Default value is `true`.
+
+.. option:: AllowMemberFunctionsInClass
+
+  When `true`, only methods defined in anonymous namespace outside of the
+  corresponding class will be warned. Default value is `true`.

clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace-allow-member-functions.cpp

@@ -0,0 +1,65 @@
+// RUN: %check_clang_tidy %s llvm-prefer-static-over-anonymous-namespace %t -- \
+// RUN:   -config="{CheckOptions: { \
+// RUN:     llvm-prefer-static-over-anonymous-namespace.AllowMemberFunctionsInClass: false }, \
+// RUN:   }" -- -fno-delayed-template-parsing
+
+namespace {
+
+class MyClass {
+public:
+  MyClass() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: place definition of method 'MyClass' outside of an anonymous namespace
+  MyClass(const MyClass&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: place definition of method 'MyClass' outside of an anonymous namespace
+  MyClass& operator=(const MyClass&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: place definition of method 'operator=' outside of an anonymous namespace
+  MyClass& operator=(MyClass&&) { return *this; };
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: place definition of method 'operator=' outside of an anonymous namespace
+  bool operator<(const MyClass&) const { return true; };
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: place definition of method 'operator<' outside of an anonymous namespace
+  void memberFunction();
+  void memberDefinedInClass() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: place definition of method 'memberDefinedInClass' outside of an anonymous namespace
+};
+
+void MyClass::memberFunction() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: place definition of method 'memberFunction' outside of an anonymous namespace
+
+template<typename T>
+class TemplateClass {
+public:
+  TemplateClass() {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: place definition of method 'TemplateClass<T>' outside of an anonymous namespace
+  TemplateClass(const TemplateClass&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: place definition of method 'TemplateClass<T>' outside of an anonymous namespace
+  TemplateClass& operator=(const TemplateClass&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: place definition of method 'operator=' outside of an anonymous namespace
+  TemplateClass& operator=(TemplateClass&&) { return *this; };
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: place definition of method 'operator=' outside of an anonymous namespace
+  bool operator<(const TemplateClass&) const { return true; };
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: place definition of method 'operator<' outside of an anonymous namespace
+  void memberFunc();
+  T getValue() const { return {}; };
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: place definition of method 'getValue' outside of an anonymous namespace
+  void memberDefinedInClass() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: place definition of method 'memberDefinedInClass' outside of an anonymous namespace
+};
+
+template<typename T>
+void TemplateClass<T>::memberFunc() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: place definition of method 'memberFunc' outside of an anonymous namespace
+
+class OuterClass {
+public:
+  class NestedClass {
+  public:
+    void nestedMemberFunc();
+    void nestedMemberDefinedInClass() {}
+    // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: place definition of method 'nestedMemberDefinedInClass' outside of an anonymous namespace
+  };
+};
+
+void OuterClass::NestedClass::nestedMemberFunc() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: place definition of method 'nestedMemberFunc' outside of an anonymous namespace
+
+} // namespace

clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace-allow-variables.cpp

@@ -0,0 +1,60 @@
+// RUN: %check_clang_tidy %s llvm-prefer-static-over-anonymous-namespace %t -- \
+// RUN:   -config="{CheckOptions: { \
+// RUN:     llvm-prefer-static-over-anonymous-namespace.AllowVariableDeclarations: false }, \
+// RUN:   }" -- -fno-delayed-template-parsing
+
+namespace {
+
+void regularFunction(int param) {
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'regularFunction' is declared in an anonymous namespace;
+
+  int Variable = 42;
+  auto Lambda = []() { return 42; };
+  static int StaticVariable = 42;
+}
+
+int globalVariable = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'globalVariable' is declared in an anonymous namespace;
+
+static int staticVariable = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: place static variable 'staticVariable' outside of an anonymous namespace
+
+typedef int MyInt;
+const MyInt myGlobalVariable = 42;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: variable 'myGlobalVariable' is declared in an anonymous namespace;
+
+template<typename T>
+constexpr T Pi = T(3.1415926);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: variable 'Pi' is declared in an anonymous namespace;
+
+void (*funcPtr)() = nullptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: variable 'funcPtr' is declared in an anonymous namespace;
+
+auto lambda = []() { return 42; };
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: variable 'lambda' is declared in an anonymous namespace;
+
+class MyClass {
+  int member;
+};
+
+MyClass instance;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'instance' is declared in an anonymous namespace;
+
+MyClass* instancePtr = nullptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: variable 'instancePtr' is declared in an anonymous namespace;
+
+MyClass& instanceRef = instance;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: variable 'instanceRef' is declared in an anonymous namespace;
+
+class OtherClass{
+  void method() {
+    MyClass instance;
+    MyClass* instancePtr = nullptr;
+    MyClass& instanceRef = instance;
+  }
+  MyClass member;
+  MyClass* memberPtr = nullptr;
+  MyClass& memberRef = member;
+};
+
+} // namespace