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)) {
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)) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
