llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Baranov Victor (vbvictor) <details> <summary>Changes</summary> Finds function and variable declarations inside anonymous namespace and suggests replacing them with ``static`` declarations. The check will enforce that [restrict-visibility](https://llvm.org/docs/CodingStandards.html#restrict-visibility) rule in LLVM Coding Standards is followed correctly (by adding `static` to functions instead of putting them in anonimous namespace). The check has additional levels of "strictness" represented by Options. By default, the check works in the most relaxed way by giving warning only for methods and functions defined in anonymous namespaces. Also, It finds `static` functions that are placed inside anonymous namespace - there is no point in keeping them inside. --- Patch is 23.08 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/142839.diff 10 Files Affected: - (modified) clang-tools-extra/clang-tidy/llvm/CMakeLists.txt (+1) - (modified) clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp (+3) - (added) clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.cpp (+96) - (added) clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.h (+42) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/llvm/prefer-static-over-anonymous-namespace.rst (+73) - (added) clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace-allow-member-functions.cpp (+65) - (added) clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace-allow-variables.cpp (+60) - (added) clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace.cpp (+146) ``````````diff diff --git a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt index 79c58a19aedac..3232f6e2cafe5 100644 --- a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt +++ b/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 diff --git a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp index e749163699b34..854e32b5ec514 100644 --- a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp +++ b/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"); diff --git a/clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.cpp new file mode 100644 index 0000000000000..f24504619b458 --- /dev/null +++ b/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 diff --git a/clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.h b/clang-tools-extra/clang-tidy/llvm/PreferStaticOverAnonymousNamespaceCheck.h new file mode 100644 index 0000000000000..ca0245e1d3031 --- /dev/null +++ b/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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e0f81a032c38d..17142b5a0eaec 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -136,6 +136,12 @@ New checks Finds unintended character output from ``unsigned char`` and ``signed char`` to an ``ostream``. +- 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:`portability-avoid-pragma-once <clang-tidy/checks/portability/avoid-pragma-once>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 5a79d61b1fd7e..7f6564af08fff 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -249,6 +249,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>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/llvm/prefer-static-over-anonymous-namespace.rst b/clang-tools-extra/docs/clang-tidy/checks/llvm/prefer-static-over-anonymous-namespace.rst new file mode 100644 index 0000000000000..21ba161189548 --- /dev/null +++ b/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`. \ No newline at end of file diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace-allow-member-functions.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace-allow-member-functions.cpp new file mode 100644 index 0000000000000..d31f56c603631 --- /dev/null +++ b/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 diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace-allow-variables.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace-allow-variables.cpp new file mode 100644 index 0000000000000..7efd872bcf352 --- /dev/null +++ b/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 diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace.cpp new file mode 100644 index 0000000000000..2dfa82717b5f7 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-static-over-anonymous-namespace.cpp @@ -0,0 +1,146 @@ +// RUN: %check_clang_tidy %s llvm-prefer-static-over-anonymous-namespace %t -- -- -fno-delayed-template-parsing + +namespace { + +void regularFunction() {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'regularFunction' is declared in an anonymous namespace; prefer using 'static' for restricting visibility [llvm-prefer-static-over-anonymous-namespace] + +void declaredFunction(); + +static void staticFunction() {} +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: place static function 'staticFunction' outside of an anonymous namespace + +int globalVariable = 42; + +static int staticVariable = 42; + +class MyClass { +public: + MyClass(); + MyClass(const MyClass&) {} + MyClass(MyClass&&) = default; + MyClass& operator=(const MyClass&); + MyClass& operator=(MyClass&&); + bool operator<(const MyClass&) const; + void memberFunction(); + static void staticMemberFunction(); + void memberDefinedInClass() {} + static void staticMemberDefinedInClass() {} +}; + +MyClass::MyClass() {} +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: place definition of method 'MyClass' outside of an anonymous namespace + +MyClass& MyClass::operator=(const MyClass&) { return *this; } +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: place definition of method 'operator=' outside of an anonymous namespace + +MyClass& MyClass::operator=(MyClass&&) = default; +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: place definition of method 'operator=' outside of an anonymous namespace + +bool MyClass::operator<(const MyClass&) const { return true; } +// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: place definition of method 'operator<' outside of an anonymous namespace + +void MyClass::memberFunction() {} +// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: place definition of method 'memberFunction' outside of an anonymous namespace + +void MyClass::staticMemberFunction() {} +// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: place definition of method 'staticMemberFunction' outside of an anonymous namespace + +template<typename ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/142839 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
