[libc] Update libc namespace clang-tidy checks #98424
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Namespace macro that should be used to declare a new namespace is updated from LIBC_NAMESPACE to LIBC_NAMESPACE_DECL which by default has hidden visibility (llvm#97109). This commit updates the clang-tidy checks to match the new policy.
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Paul Kirth (ilovepi) ChangesThis patch updates the clang-tidy checks for llvm-libc to ensure that the namespace macro used to declare the libc namespace is updated from LIBC_NAMESPACE to LIBC_NAMESPACE_DECL which by default has hidden visibility. Co-authored-by: Prabhu Rajesakeran <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/98424.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
index af977bff70f7c..caa19c595823f 100644
--- a/clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
@@ -51,7 +51,7 @@ void CalleeNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
// __llvm_libc, we're good.
const auto *NS = dyn_cast<NamespaceDecl>(getOutermostNamespace(FuncDecl));
if (NS && Result.SourceManager->isMacroBodyExpansion(NS->getLocation()) &&
- NS->getName().starts_with(RequiredNamespaceStart))
+ NS->getName().starts_with(RequiredNamespaceRefStart))
return;
const DeclarationName &Name = FuncDecl->getDeclName();
@@ -62,7 +62,7 @@ void CalleeNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
diag(UsageSiteExpr->getBeginLoc(),
"%0 must resolve to a function declared "
"within the namespace defined by the '%1' macro")
- << FuncDecl << RequiredNamespaceMacroName;
+ << FuncDecl << RequiredNamespaceRefMacroName;
diag(FuncDecl->getLocation(), "resolves to this declaration",
clang::DiagnosticIDs::Note);
diff --git a/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
index ae9819ed97502..bb436e4d12a30 100644
--- a/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
@@ -30,20 +30,35 @@ void ImplementationInNamespaceCheck::check(
const auto *MatchedDecl =
Result.Nodes.getNodeAs<Decl>("child_of_translation_unit");
const auto *NS = dyn_cast<NamespaceDecl>(MatchedDecl);
+
+ // LLVM libc declarations should be inside of a non-anonymous namespace.
if (NS == nullptr || NS->isAnonymousNamespace()) {
diag(MatchedDecl->getLocation(),
"declaration must be enclosed within the '%0' namespace")
- << RequiredNamespaceMacroName;
+ << RequiredNamespaceDeclMacroName;
return;
}
+
+ // Enforce that the namespace is the result of macro expansion
if (Result.SourceManager->isMacroBodyExpansion(NS->getLocation()) == false) {
diag(NS->getLocation(), "the outermost namespace should be the '%0' macro")
- << RequiredNamespaceMacroName;
+ << RequiredNamespaceDeclMacroName;
return;
}
- if (NS->getName().starts_with(RequiredNamespaceStart) == false) {
+
+ // We want the macro to have [[gnu::visibility("hidden")]] as a prefix, but
+ // visibility is just an attribute in the AST construct, so we check that
+ // instead.
+ if (NS->getVisibility() != Visibility::HiddenVisibility) {
diag(NS->getLocation(), "the '%0' macro should start with '%1'")
- << RequiredNamespaceMacroName << RequiredNamespaceStart;
+ << RequiredNamespaceDeclMacroName << RequiredNamespaceDeclStart;
+ return;
+ }
+
+ // Lastly, make sure the namespace name actually has the __llvm_libc prefix
+ if (NS->getName().starts_with(RequiredNamespaceRefStart) == false) {
+ diag(NS->getLocation(), "the '%0' macro expansion should start with '%1'")
+ << RequiredNamespaceDeclMacroName << RequiredNamespaceRefStart;
return;
}
}
diff --git a/clang-tools-extra/clang-tidy/llvmlibc/NamespaceConstants.h b/clang-tools-extra/clang-tidy/llvmlibc/NamespaceConstants.h
index 7d4120085b866..83908a7875d03 100644
--- a/clang-tools-extra/clang-tidy/llvmlibc/NamespaceConstants.h
+++ b/clang-tools-extra/clang-tidy/llvmlibc/NamespaceConstants.h
@@ -10,7 +10,11 @@
namespace clang::tidy::llvm_libc {
-const static llvm::StringRef RequiredNamespaceStart = "__llvm_libc";
-const static llvm::StringRef RequiredNamespaceMacroName = "LIBC_NAMESPACE";
+const static llvm::StringRef RequiredNamespaceRefStart = "__llvm_libc";
+const static llvm::StringRef RequiredNamespaceRefMacroName = "LIBC_NAMESPACE";
+const static llvm::StringRef RequiredNamespaceDeclStart =
+ "[[gnu::visibility(\"hidden\")]] __llvm_libc";
+const static llvm::StringRef RequiredNamespaceDeclMacroName =
+ "LIBC_NAMESPACE_DECL";
} // namespace clang::tidy::llvm_libc
diff --git a/clang-tools-extra/docs/clang-tidy/checks/llvmlibc/implementation-in-namespace.rst b/clang-tools-extra/docs/clang-tidy/checks/llvmlibc/implementation-in-namespace.rst
index 47ea2b866a934..ec52b9f73a3f3 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/llvmlibc/implementation-in-namespace.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/llvmlibc/implementation-in-namespace.rst
@@ -8,13 +8,13 @@ correct namespace.
.. code-block:: c++
- // Implementation inside the LIBC_NAMESPACE namespace.
+ // Implementation inside the LIBC_NAMESPACE_DECL namespace.
// Correct if:
- // - LIBC_NAMESPACE is a macro
- // - LIBC_NAMESPACE expansion starts with `__llvm_libc`
- namespace LIBC_NAMESPACE {
+ // - LIBC_NAMESPACE_DECL is a macro
+ // - LIBC_NAMESPACE_DECL expansion starts with `[[gnu::visibility("hidden")]] __llvm_libc`
+ namespace LIBC_NAMESPACE_DECL {
void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
- // Namespaces within LIBC_NAMESPACE namespace are allowed.
+ // Namespaces within LIBC_NAMESPACE_DECL namespace are allowed.
namespace inner {
int localVar = 0;
}
@@ -22,16 +22,16 @@ correct namespace.
extern "C" void str_fuzz() {}
}
- // Incorrect: implementation not in the LIBC_NAMESPACE namespace.
+ // Incorrect: implementation not in the LIBC_NAMESPACE_DECL namespace.
void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
- // Incorrect: outer most namespace is not the LIBC_NAMESPACE macro.
+ // Incorrect: outer most namespace is not the LIBC_NAMESPACE_DECL macro.
namespace something_else {
void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
}
- // Incorrect: outer most namespace expansion does not start with `__llvm_libc`.
- #define LIBC_NAMESPACE custom_namespace
- namespace LIBC_NAMESPACE {
+ // Incorrect: outer most namespace expansion does not start with `[[gnu::visibility("hidden")]] __llvm_libc`.
+ #define LIBC_NAMESPACE_DECL custom_namespace
+ namespace LIBC_NAMESPACE_DECL {
void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvmlibc/implementation-in-namespace.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvmlibc/implementation-in-namespace.cpp
index 2246a430dd1f5..4a5576f2c5d62 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/llvmlibc/implementation-in-namespace.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/llvmlibc/implementation-in-namespace.cpp
@@ -3,60 +3,61 @@
#define MACRO_A "defining macros outside namespace is valid"
class ClassB;
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be enclosed within the 'LIBC_NAMESPACE' namespace
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be enclosed within the 'LIBC_NAMESPACE_DECL' namespace
struct StructC {};
-// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration must be enclosed within the 'LIBC_NAMESPACE' namespace
-char *VarD = MACRO_A;
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be enclosed within the 'LIBC_NAMESPACE' namespace
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration must be enclosed within the 'LIBC_NAMESPACE_DECL' namespace
+const char *VarD = MACRO_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: declaration must be enclosed within the 'LIBC_NAMESPACE_DECL' namespace
typedef int typeE;
-// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: declaration must be enclosed within the 'LIBC_NAMESPACE' namespace
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: declaration must be enclosed within the 'LIBC_NAMESPACE_DECL' namespace
void funcF() {}
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: declaration must be enclosed within the 'LIBC_NAMESPACE' namespace
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: declaration must be enclosed within the 'LIBC_NAMESPACE_DECL' namespace
namespace outer_most {
-// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the outermost namespace should be the 'LIBC_NAMESPACE' macro
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the outermost namespace should be the 'LIBC_NAMESPACE_DECL' macro
class A {};
}
// Wrapped in anonymous namespace.
namespace {
-// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: declaration must be enclosed within the 'LIBC_NAMESPACE' namespace
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: declaration must be enclosed within the 'LIBC_NAMESPACE_DECL' namespace
class A {};
}
namespace namespaceG {
-// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the outermost namespace should be the 'LIBC_NAMESPACE' macro
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the outermost namespace should be the 'LIBC_NAMESPACE_DECL' macro
namespace __llvm_libc {
namespace namespaceH {
class ClassB;
} // namespace namespaceH
struct StructC {};
} // namespace __llvm_libc
-char *VarD = MACRO_A;
+const char *VarD = MACRO_A;
typedef int typeE;
void funcF() {}
} // namespace namespaceG
// Wrapped in macro namespace but with an incorrect name
-#define LIBC_NAMESPACE custom_namespace
-namespace LIBC_NAMESPACE {
-// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the 'LIBC_NAMESPACE' macro should start with '__llvm_libc'
+#define LIBC_NAMESPACE_DECL [[gnu::visibility("hidden")]] custom_namespace
+namespace LIBC_NAMESPACE_DECL {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the 'LIBC_NAMESPACE_DECL' macro expansion should start with '__llvm_libc'
+
namespace namespaceH {
class ClassB;
} // namespace namespaceH
-} // namespace LIBC_NAMESPACE
+} // namespace LIBC_NAMESPACE_DECL
-// Wrapped in macro namespace with a valid name, LIBC_NAMESPACE starts with '__llvm_libc'
-#undef LIBC_NAMESPACE
-#define LIBC_NAMESPACE __llvm_libc_xyz
-namespace LIBC_NAMESPACE {
+// Wrapped in macro namespace with a valid name, LIBC_NAMESPACE_DECL starts with '__llvm_libc'
+#undef LIBC_NAMESPACE_DECL
+#define LIBC_NAMESPACE_DECL [[gnu::visibility("hidden")]] __llvm_libc_xyz
+namespace LIBC_NAMESPACE_DECL {
namespace namespaceI {
class ClassB;
} // namespace namespaceI
struct StructC {};
-char *VarD = MACRO_A;
+const char *VarD = MACRO_A;
typedef int typeE;
void funcF() {}
extern "C" void extern_funcJ() {}
-} // namespace LIBC_NAMESPACE
+} // namespace LIBC_NAMESPACE_DECL
|
ilovepi
commented
Jul 11, 2024
| char *VarD = MACRO_A; | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be enclosed within the 'LIBC_NAMESPACE' namespace | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration must be enclosed within the 'LIBC_NAMESPACE_DECL' namespace | ||
| const char *VarD = MACRO_A; |
There was a problem hiding this comment.
I know this is a bit unrelated, but the tidy output was pretty polluted with warnings due to the lack of const here. When I manually ran the tidy command + Filecheck, it seemed to fail due to those warnings, so I just fixed those since I was changing these checks anyway.
EugeneZelenko
requested review from
PiotrZSL,
SimplyDanny,
5chmidti and
HerrCai0907
PiotrZSL
reviewed
Jul 11, 2024
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
Show resolved
Hide resolved
ilovepi
commented
Jul 11, 2024
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
Show resolved
Hide resolved
|
Also, as an FYI to reviewers, I'm working on the FixIts for these as a separate patch. |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
This patch updates the clang-tidy checks for llvm-libc to ensure that the namespace macro used to declare the libc namespace is updated from LIBC_NAMESPACE to LIBC_NAMESPACE_DECL which by default has hidden visibility. Co-authored-by: Prabhu Rajesakeran <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch updates the clang-tidy checks for llvm-libc to ensure that the namespace macro used to declare the libc namespace is updated from LIBC_NAMESPACE to LIBC_NAMESPACE_DECL which by default has hidden visibility.
Co-authored-by: Prabhu Rajesakeran [email protected]