[clang-tidy] Do not warn about redundant static in misc-use-anonymous-namespace

[carlosgalvezp]

Summary

The same functionality is already implemented in the
readability-static-definition-in-anonymous-namespace
check, including automatic fixes.

[carlosgalvezp]

Reorder matchers to reduce diff.

[lebedev.ri]

This patch should not land before that one does.

[carlosgalvezp]

In D139197#3966549, @lebedev.ri wrote:

This patch should not land before that one does.

The original code is 1 day old. Do we really need to be so strict?

[carlosgalvezp]

My plan was to apply this removal in the same patch where I add the new check, but I know I'd get comments like "please do the removal in a separate patch". So that's what I'm doing :)

[lebedev.ri]

I was commenting because we already have a similar diagnostic in clang-tidy 15:

namespace {
  static void foo();
  static void bar(){}
  void foo() {}
  static int x;
}

$ clang-tidy-15  -checks=\*,-llvmlibc-* /tmp/test.cpp 
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "/tmp/test.cpp"
No compilation database found in /tmp or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
5 warnings generated.
/tmp/test.cpp:3:17: warning: 'bar' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
    static void bar(){}
    ~~~~~~~     ^
/tmp/test.cpp:5:16: warning: variable 'x' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
    static int x;
               ^
/tmp/test.cpp:5:16: warning: variable name 'x' is too short, expected at least 3 characters [readability-identifier-length]
/tmp/test.cpp:5:16: warning: 'x' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
    static int x;
    ~~~~~~~    ^
/tmp/test.cpp:6:3: warning: anonymous namespace not terminated with a closing comment [llvm-namespace-comment]
  }
  ^
    // namespace
/tmp/test.cpp:1:13: note: anonymous namespace starts here
  namespace {
            ^

that for some reason only fires on definitions, not declarations.
I thought this was changing that old check.
I think it may be good to not have duplicate coverage,
but do we need a new check given that there is one already?

[carlosgalvezp]

Ah, then it has the exact same functionality! My idea for readability-redundant-static was to warn about:

• static in anonymous namespace
• static const/constexpr at global scope (since const gives implicit internal linkage in C++).

I will see what I can do about that.

This shouldn't be a blocker for this patch though, it's just removing functionality that belongs in another check. As said, the check is 1 day old so it's unlikely anyone is relying on it yet :)

[carlosgalvezp]

Update commit message.

[carlosgalvezp]

Simplify anonymous namespace matcher.

[lebedev.ri]

This shouldn't be a blocker for this patch though, it's just removing functionality that belongs in another check. As said, the check is 1 day old so it's unlikely anyone is relying on it yet :)

SGTM in general.

[carlosgalvezp]

@lebedev.ri If you are happy with the patch, would you mind approving it? Or do you have additional comments that should be addressed? The patch does not need to be approved by the Code Owner.

[lebedev.ri]

In D139197#3977370, @carlosgalvezp wrote:

@lebedev.ri If you are happy with the patch, would you mind approving it? Or do you have additional comments that should be addressed? The patch does not need to be approved by the Code Owner.

No further comments. Thanks.

[carlosgalvezp]

In D139197#3978174, @lebedev.ri wrote:

In D139197#3977370, @carlosgalvezp wrote:

@lebedev.ri If you are happy with the patch, would you mind approving it? Or do you have additional comments that should be addressed? The patch does not need to be approved by the Code Owner.

No further comments. Thanks.

Ops, I realize you weren't in the reviewers list, I added you now.

[carlosgalvezp]

@njames93 @JonasToth @LegalizeAdulthood Do you have any comments that I should address? If not, could you approve the patch?

To recap: this check is a few days old (I added it myself). I realized there's functionality that shouldn't be here so I'm just removing it. Should be a fairly safe change.

[aaron.ballman]

LGTM, though please add a release note describing the change.

[carlosgalvezp]

In D139197#3978488, @aaron.ballman wrote:

LGTM, though please add a release note describing the change

The original check is just a few days old. It didn't exist in the previous release. Should I still add a comment about this change?

[carlosgalvezp]

PS thanks a lot for the review! :)

[aaron.ballman]

In D139197#3978761, @carlosgalvezp wrote:

In D139197#3978488, @aaron.ballman wrote:

LGTM, though please add a release note describing the change

The original check is just a few days old. It didn't exist in the previous release. Should I still add a comment about this change?

Oh, I hadn't realized the original check was part of this release. No need for a release note in this case.

In D139197#3978771, @carlosgalvezp wrote:

PS thanks a lot for the review! :)

Happy to help where I can!

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.
+/// suggests moving them to 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 {

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

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;
+AST_MATCHER(Decl, isInAnonymousNamespace) {
+  return Node.isInAnonymousNamespace();
 }
+} // namespace
 
 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);
+      functionDecl(isStatic(),
+                   unless(anyOf(isInAnonymousNamespace(), isMemberFunction())))
+          .bind("func"),
+      this);
   Finder->addMatcher(
-      varDecl(isStatic(), unless(anyOf(isStaticLocal(), isStaticDataMember())))
+      varDecl(isStatic(), unless(anyOf(isInAnonymousNamespace(),
+                                       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/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``.
+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.
 
 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;