[clang-tidy] Fix a couple additional cases in misc-use-anonymous-namespace

[carlosgalvezp]

Summary

• Do not analyze header files, since we don't want to promote using anonymous namespaces there.

• Do not warn about const/constexpr variables, those are implicitly static in C++ and they don't need to be moved to an anonymous namespace. Warning about redundant static in general could be implemented as a standalone check, moving away some of the functionality from this check.

[carlosgalvezp]

Fix naming convention

[njames93]

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

40

The logic would fall apart when run in clangd or unity builds. The safest way is generally to match the file extension, we have a class for it in clang-tidy/utils

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

35

These changes just look like noise.

[carlosgalvezp]

Address comments

[carlosgalvezp]

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

40

Great suggestion, thanks!

[carlosgalvezp]

Add options for users to define their own header file
extensions.

[carlosgalvezp]

Document ignored cases in the documentation.

[carlosgalvezp]

Remove accidentally added newline.

[carlosgalvezp]

I'm happy with my changes now, ready for a new round of review! :)

[tschuett]

The LLVM coding style says to prefer static over anonymous namespaces.

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

17

extensions

[carlosgalvezp]

Fix typo

[carlosgalvezp]

The LLVM coding style says to prefer static over anonymous namespaces.

Yes, but this is not an LLVM check, it's a misc check. Other projects are allowed to have other guidelines.

[carlosgalvezp]

Fix typo in commit message

[carlosgalvezp]

Rebase

[carlosgalvezp]

Friendly ping.

[vingeldal]

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

46–48

Is it really the best behavior to allow these? If I got the rationale right we don't warn about this because const and constexpr have implicit internal linkage anyway, so static doesn't make a difference, right?
Reading the documentation for this check I gather static would probably have been deprecated if it wasn't for the fact that deprecation would have broken compatibility with C. So, if we drop the static keyword here we still get the behavior we want, without a confusing keyword we would rather get rid of if we could, while keeping compatibility with C.

I'm thinking it could be better to just discourage from using static for cases like this.

[carlosgalvezp]

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

46–48

Thanks for the review!

Yes, that example is "bad" code, but this particular check would currently ask users to move the variables to the anonymous namespace - which is also incorrect.

My plan is to add one more separate checks in the "readability" section that warn about redundant usages of 'static' (like this one), and even supports automatic fixes (which aren't possible in this check).

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"
+#include "../utils/FileExtensionsUtils.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.
 ///
+/// The check supports these options:
+///   - `HeaderFileExtensions`: a semicolon-separated list of filename
+///     extensions of header files (The filename extension should not contain
+///     "." prefix). ";h;hh;hpp;hxx" by default.
+///
+///     For extension-less header files, using an empty string or leaving an
+///     empty string between ";" if there are other filename extensions.
+///
 /// 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) {}
+  UseAnonymousNamespaceCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const StringRef RawStringHeaderFileExtensions;
+  utils::FileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEANONYMOUSNAMESPACECHECK_H

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

 namespace tidy {
 namespace misc {
 namespace {
 AST_POLYMORPHIC_MATCHER(isStatic, AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
                                                                   VarDecl)) {
   return Node.getStorageClass() == SC_Static;
 }
 
+AST_POLYMORPHIC_MATCHER_P(isInHeaderFile,
+                          AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+                                                          VarDecl),
+                          utils::FileExtensionsSet, HeaderFileExtensions) {
+  return utils::isExpansionLocInHeaderFile(
+      Node.getBeginLoc(), Finder->getASTContext().getSourceManager(),
+      HeaderFileExtensions);
+}
+
 AST_MATCHER(FunctionDecl, isMemberFunction) {
   return llvm::isa<CXXMethodDecl>(&Node);
 }
 AST_MATCHER(VarDecl, isStaticDataMember) { return Node.isStaticDataMember(); }
 
 AST_MATCHER(Decl, isInAnonymousNamespace) {
   return Node.isInAnonymousNamespace();
 }
 } // namespace
 
+UseAnonymousNamespaceCheck::UseAnonymousNamespaceCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
+          "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
+  if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
+                                  HeaderFileExtensions,
+                                  utils::defaultFileExtensionDelimiters())) {
+    this->configurationDiag("Invalid header file extension: '%0'")
+        << RawStringHeaderFileExtensions;
+  }
+}
+
+void UseAnonymousNamespaceCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+}
+
 void UseAnonymousNamespaceCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       functionDecl(isStatic(),
-                   unless(anyOf(isInAnonymousNamespace(), isMemberFunction())))
+                   unless(anyOf(isInHeaderFile(HeaderFileExtensions),
+                                isInAnonymousNamespace(), isMemberFunction())))
           .bind("x"),
       this);
   Finder->addMatcher(
-      varDecl(isStatic(), unless(anyOf(isInAnonymousNamespace(),
-                                       isStaticLocal(), isStaticDataMember())))
+      varDecl(isStatic(),
+              unless(anyOf(isInHeaderFile(HeaderFileExtensions),
+                           isInAnonymousNamespace(), isStaticLocal(),
+                           isStaticDataMember(), hasType(isConstQualified()))))
           .bind("x"),
       this);
 }
 
 void UseAnonymousNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *MatchedDecl = Result.Nodes.getNodeAs<NamedDecl>("x")) {
     StringRef Type = llvm::isa<VarDecl>(MatchedDecl) ? "variable" : "function";
     diag(MatchedDecl->getLocation(),
          "%0 %1 declared 'static', move to anonymous namespace instead")
         << Type << MatchedDecl;
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

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.
 
 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.
 
+The following uses of ``static`` will *not* be diagnosed:
+
+* Functions or variables in header files, since anonymous namespaces in headers
+  is considered an antipattern. Allowed header file extensions can be configured
+  via the `HeaderFileExtensions` option (see below).
+* ``const`` or ``constexpr`` variables, since they already have implicit internal
+  linkage in C++.
+
 Examples:
 
 .. code-block:: c++
 
   // Bad
   static void foo();
   static int x;
 
   // Good
   namespace {
     void foo();
     int x;
   } // namespace
 
+Options
+-------
+
+.. option:: HeaderFileExtensions
+
+   A semicolon-separated list of filename extensions of header files (the filename
+   extensions should not include "." prefix). Default is ";h;hh;hpp;hxx".
+   For extension-less header files, using an empty string or leaving an
+   empty string between ";" if there are other filename extensions.
+
 [1] `Undeprecating static <https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1012>`_

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

+// Should not warn here, do not require anonymous namespaces in headers
+static int gv{123};
+static void gf(){}

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

-// RUN: %check_clang_tidy %s misc-use-anonymous-namespace %t
+// RUN: %check_clang_tidy %s misc-use-anonymous-namespace %t -- -header-filter=.* -- -I%S/Inputs
+#include "use-anonymous-namespace.h"
 
 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 a {
   static void f3();

 namespace {
   void f7();
   int v7;
 }
 }
 
 // OK
 struct Foo {
   static void f();
   static int x;
 };
 
 // OK
 void foo()
 {
   static int x;
 }
+
+// OK
+static const int v8{123};
+static constexpr int v9{123};