[clang-tidy] Add misc-use-anonymous-namespace check

[carlosgalvezp]

Fix release notes.

[carlosgalvezp]

Add reference to undeprecation of static.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/modernize/UseAnonymousNamespaceCheck.cpp

31 ↗

(On Diff #472948)

Please don't use auto unless type is explicitly spelled in same statement or iterator.

[carlosgalvezp]

Address review comments.

[njames93]

I'm not entirely sure why this belongs in the modernize module given anonymous namespaces have been in c++ forever, maybe its more of a misc check? Also the modernize checks are meant to actually emit fixes(ignore the c arrays check :) ), Right now, this only warn, it doesn't appear to act

[carlosgalvezp]

In D137340#3906250, @njames93 wrote:

I'm not entirely sure why this belongs in the modernize module given anonymous namespaces have been in c++ forever, maybe its more of a misc check? Also the modernize checks are meant to actually emit fixes(ignore the c arrays check :) ), Right now, this only warn, it doesn't appear to act

Don't have a strong opinion, if you want I can move it there! I put in modernize as in "modernize from C-style into C++ style". But I know there's diverse opinions about this (LLVM not following this, for example) so perhaps it should be misc. Let me know which one you'd prefer so I can apply the change!

I didn't add fix because it would add quite some complexity, e.g. figuring out if there's is already an anon namespace to move into, or create a new one, users might want configurable behavior, etc.

[carlosgalvezp]

Move to misc

[carlosgalvezp]

Update commit message

[carlosgalvezp]

Remove some code duplication

[carlosgalvezp]

Fix typo in binding names

[carlosgalvezp]

Simplify code for printing the type of the declaration.

[carlosgalvezp]

@njames93 Friendly ping

[carlosgalvezp]

@njames93 Ping again. Alternatively, could someone else review? @aaron.ballman maybe?

[carlosgalvezp]

Another week, another ping @njames93 @aaron.ballman

I have addressed all comments since 3 weeks ago, and have not received any objections. I intend to land this patch next week (1st December) if I don't receive any further feedback.

[carlosgalvezp]

Replace "the" anonymous namespace with "an" anonymous namespace
Rebase.

[bigdavedev]

clang-tools-extra/docs/clang-tidy/checks/misc/use-anonymous-namespace.rst

8–10

Would we want this to be a fix as well? Removing the static keyword automatically would be nice.

[carlosgalvezp]

clang-tools-extra/docs/clang-tidy/checks/misc/use-anonymous-namespace.rst

8–10

Would make sense to fix, but unfortunately with my limited AST knowledge I don't know how to get the location of the static token to apply a removal. So unless someone wants to help out with that it will have to stay as is for now.

[bigdavedev]

Looks good to me. All comments/suggestions/concerns addressed.

clang-tools-extra/docs/clang-tidy/checks/misc/use-anonymous-namespace.rst

8–10

Ack.

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

add_clang_library(clangTidyMiscModule
   RedundantExpressionCheck.cpp
   StaticAssertCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   UniqueptrResetReleaseCheck.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedUsingDeclsCheck.cpp
+  UseAnonymousNamespaceCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangTidy
   clangTidyUtils
 
   DEPENDS
   omp_gen

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

 #include "RedundantExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedUsingDeclsCheck.h"
+#include "UseAnonymousNamespaceCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
         "misc-uniqueptr-reset-release");
     CheckFactories.registerCheck<UnusedAliasDeclsCheck>(
         "misc-unused-alias-decls");
     CheckFactories.registerCheck<UnusedParametersCheck>(
         "misc-unused-parameters");
     CheckFactories.registerCheck<UnusedUsingDeclsCheck>(
         "misc-unused-using-decls");
+    CheckFactories.registerCheck<UseAnonymousNamespaceCheck>(
+        "misc-use-anonymous-namespace");
   }
 };
 
 } // namespace misc
 
 // Register the MiscTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<misc::MiscModule>
     X("misc-module", "Adds miscellaneous lint checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the MiscModule.
 volatile int MiscModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/misc/UseAnonymousNamespaceCheck.h

+//===--- UseAnonymousNamespaceCheck.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_MISC_USEANONYMOUSNAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEANONYMOUSNAMESPACECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Warns when using 'static' functions or variables at global scope, and
+/// suggests moving them to an anonymous namespace. It also suggests removing
+/// 'static' if they are already inside an anonymous namespace.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-anonymous-namespace.html
+class UseAnonymousNamespaceCheck : public ClangTidyCheck {
+public:
+  UseAnonymousNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  template <typename T> void processMatch(const T *MatchedDecl);
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEANONYMOUSNAMESPACECHECK_H

clang-tools-extra/clang-tidy/misc/UseAnonymousNamespaceCheck.cpp

+//===--- UseAnonymousNamespaceCheck.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 "UseAnonymousNamespaceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+namespace {
+AST_POLYMORPHIC_MATCHER(isStatic, AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+                                                                  VarDecl)) {
+  return Node.getStorageClass() == SC_Static;
+}
+
+AST_MATCHER(FunctionDecl, isMemberFunction) {
+  return llvm::isa<CXXMethodDecl>(&Node);
+}
+AST_MATCHER(VarDecl, isStaticDataMember) { return Node.isStaticDataMember(); }
+} // namespace
+
+static bool isInAnonymousNamespace(const Decl *Decl) {
+  const DeclContext *DC = Decl->getDeclContext();
+  if (DC && DC->isNamespace()) {
+    const auto *ND = llvm::cast<NamespaceDecl>(DC);
+    if (ND && ND->isAnonymousNamespace())
+      return true;
+  }
+  return false;
+}
+
+template <typename T>
+void UseAnonymousNamespaceCheck::processMatch(const T *MatchedDecl) {
+  StringRef Type = llvm::isa<VarDecl>(MatchedDecl) ? "variable" : "function";
+  if (isInAnonymousNamespace(MatchedDecl))
+    diag(MatchedDecl->getLocation(), "%0 %1 declared 'static' in "
+                                     "anonymous namespace, remove 'static'")
+        << Type << MatchedDecl;
+  else
+    diag(MatchedDecl->getLocation(),
+         "%0 %1 declared 'static', move to anonymous namespace instead")
+        << Type << MatchedDecl;
+}
+
+void UseAnonymousNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      functionDecl(isStatic(), unless(isMemberFunction())).bind("func"), this);
+  Finder->addMatcher(
+      varDecl(isStatic(), unless(anyOf(isStaticLocal(), isStaticDataMember())))
+          .bind("var"),
+      this);
+}
+
+void UseAnonymousNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("func"))
+    processMatch(MatchedDecl);
+
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("var"))
+    processMatch(MatchedDecl);
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
 - New :doc:`cppcoreguidelines-avoid-do-while
   <clang-tidy/checks/cppcoreguidelines/avoid-do-while>` check.
 
   Warns when using ``do-while`` loops.
 
+- New :doc:`misc-use-anonymous-namespace
+  <clang-tidy/checks/misc/use-anonymous-namespace>` check.
+
+  Warns when using ``static`` function or variables at global scope, and suggests
+  moving them into an anonymous namespace.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 

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

.. csv-table::
    `misc-redundant-expression <misc/redundant-expression.html>`_, "Yes"
    `misc-static-assert <misc/static-assert.html>`_, "Yes"
    `misc-throw-by-value-catch-by-reference <misc/throw-by-value-catch-by-reference.html>`_,
    `misc-unconventional-assign-operator <misc/unconventional-assign-operator.html>`_,
    `misc-uniqueptr-reset-release <misc/uniqueptr-reset-release.html>`_, "Yes"
    `misc-unused-alias-decls <misc/unused-alias-decls.html>`_, "Yes"
    `misc-unused-parameters <misc/unused-parameters.html>`_, "Yes"
    `misc-unused-using-decls <misc/unused-using-decls.html>`_, "Yes"
+   `misc-use-anonymous-namespace <misc/use-anonymous-namespace.html>`_,
    `modernize-avoid-bind <modernize/avoid-bind.html>`_, "Yes"
    `modernize-avoid-c-arrays <modernize/avoid-c-arrays.html>`_,
    `modernize-concat-nested-namespaces <modernize/concat-nested-namespaces.html>`_, "Yes"
    `modernize-deprecated-headers <modernize/deprecated-headers.html>`_, "Yes"
    `modernize-deprecated-ios-base-aliases <modernize/deprecated-ios-base-aliases.html>`_, "Yes"
    `modernize-loop-convert <modernize/loop-convert.html>`_, "Yes"
    `modernize-macro-to-enum <modernize/macro-to-enum.html>`_, "Yes"
    `modernize-make-shared <modernize/make-shared.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/misc/use-anonymous-namespace.rst

+.. title:: clang-tidy - misc-use-anonymous-namespace
+
+misc-use-anonymous-namespace
+============================
+
+Finds instances of ``static`` functions or variables declared at global scope
+that could instead be moved into an anonymous namespace. It also detects
+instances moved to an anonymous namespace that still keep the redundant
+``static``.
+
+Anonymous namespaces are the "superior alternative" according to the C++
+Standard. ``static`` was proposed for deprecation, but later un-deprecated to
+keep C compatibility [1]. ``static`` is an overloaded term with different meanings in
+different contexts, so it can create confusion.
+
+Examples:
+
+.. code-block:: c++
+
+  // Bad
+  static void foo();
+  static int x;
+
+  // Good
+  namespace {
+    void foo();
+    int x;
+  } // namespace
+
+.. code-block:: c++
+
+  // Bad
+  namespace {
+    static void foo();
+    static int x;
+  }
+
+  // Good
+  namespace {
+    void foo();
+    int x;
+  }  // namespace
+
+[1] `Undeprecating static <https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1012>`_

clang-tools-extra/test/clang-tidy/checkers/misc/use-anonymous-namespace.cpp

+// RUN: %check_clang_tidy %s misc-use-anonymous-namespace %t
+
+static void f1();
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function 'f1' declared 'static', move to anonymous namespace instead [misc-use-anonymous-namespace]
+static int v1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'v1' declared 'static', move to anonymous namespace instead
+
+namespace {
+  static void f2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: function 'f2' declared 'static' in anonymous namespace, remove 'static'
+  static int v2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: variable 'v2' declared 'static' in anonymous namespace, remove 'static'
+}
+
+namespace a {
+  static void f3();
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: function 'f3' declared 'static', move to anonymous namespace instead
+  static int v3;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: variable 'v3' declared 'static', move to anonymous namespace instead
+}
+
+namespace a {
+namespace {
+  static void f4();
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: function 'f4' declared 'static' in anonymous namespace, remove 'static'
+  static int v4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: variable 'v4' declared 'static' in anonymous namespace, remove 'static'
+}
+}
+
+// OK
+void f5();
+int v5;
+
+// OK
+namespace {
+  void f6();
+  int v6;
+}
+
+// OK
+namespace a {
+namespace {
+  void f7();
+  int v7;
+}
+}
+
+// OK
+struct Foo {
+  static void f();
+  static int x;
+};
+
+// OK
+void foo()
+{
+  static int x;
+}