[PATCH] D147379: [clang-tidy] Disable misc-definitions-in-headers for declarations in anonymous namespaces

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp created this revision.
Herald added subscribers: PiotrZSL, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Anonymous namespaces are another way of providing internal linkage,
and the check already ignores other cases of internal linkage in
headers, via "static" or "const".

Anonymous namespaces in headers are definitely a source of pitfalls,
but it's not the responsibility of this check to cover that. Instead,
google-build-namespaces will specifically warn about this issue.

Fixes https://github.com/llvm/llvm-project/issues/61471


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147379

Files:
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/misc/definitions-in-headers.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/definitions-in-headers.hpp


Index: clang-tools-extra/test/clang-tidy/checkers/misc/definitions-in-headers.hpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/misc/definitions-in-headers.hpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/definitions-in-headers.hpp
@@ -108,8 +108,7 @@
 int f5(); // OK: function declaration.
 inline int f6() { return 1; } // OK: inline function definition.
 namespace {
-  int f7() { return 1; }
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: function 'f7' defined in a header file;
+  int f7() { return 1; }  // OK: each TU defines the function in a unique namespace
 }
 
 int f8() = delete; // OK: the function being marked delete is not callable.
@@ -142,8 +141,7 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: variable 'ca' defined in a header file;
 
 namespace {
-  int e = 2;
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'e' defined in a header file;
+  int e = 2;  // OK: each TU defines the variable in a unique namespace
 }
 
 const char* const g = "foo"; // OK: internal linkage variable definition.
Index: clang-tools-extra/docs/clang-tidy/checks/misc/definitions-in-headers.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/misc/definitions-in-headers.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc/definitions-in-headers.rst
@@ -27,6 +27,7 @@
    const int c = 1;
    const char* const str2 = "foo";
    constexpr int k = 1;
+   namespace { int x = 1; }
 
    // Warning: function definition.
    int g() {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -199,6 +199,10 @@
   <clang-tidy/checks/misc/definitions-in-headers>` check.
   Global options of the same name should be used instead.
 
+- Fixed false positive in :doc:`misc-definitions-in-headers
+  <clang-tidy/checks/misc/definitions-in-headers>` to avoid warning on
+  declarations inside anonymous namespaces.
+
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`misc-unused-using-decls
   <clang-tidy/checks/misc/unused-using-decls>` check.
Index: clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
@@ -90,13 +90,11 @@
   // Internal linkage variable definitions are ignored for now:
   //   const int a = 1;
   //   static int b = 1;
+  //   namespace { int c = 1; }
   //
   // Although these might also cause ODR violations, we can be less certain and
   // should try to keep the false-positive rate down.
-  //
-  // FIXME: Should declarations in anonymous namespaces get the same treatment
-  // as static / const declarations?
-  if (!ND->hasExternalFormalLinkage() && !ND->isInAnonymousNamespace())
+  if (!ND->hasExternalFormalLinkage() || ND->isInAnonymousNamespace())
     return;
 
   if (const auto *FD = dyn_cast<FunctionDecl>(ND)) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D147379.510233.patch
Type: text/x-patch
Size: 3177 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230401/83d9ffc8/attachment.bin>