Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy][libc] Fix namespace check with macro #68134

Merged
merged 4 commits into from
Oct 6, 2023

Conversation

michaelrj-google
Copy link
Contributor

The name of the namespace for LLVM's libc is now provided by a macro.
The ImplementationNamespaceCheck was updated to handle this, but the
CalleeNamespaceCheck was missed. This patch updates the
CalleeNamespaceCheck to handle the macro.

The name of the namespace for LLVM's libc is now provided by a macro.
The ImplementationNamespaceCheck was updated to handle this, but the
CalleeNamespaceCheck was missed. This patch updates the
CalleeNamespaceCheck to handle the macro.
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 3, 2023

@llvm/pr-subscribers-clang-tidy

Changes

The name of the namespace for LLVM's libc is now provided by a macro.
The ImplementationNamespaceCheck was updated to handle this, but the
CalleeNamespaceCheck was missed. This patch updates the
CalleeNamespaceCheck to handle the macro.


Full diff: https://github.com/llvm/llvm-project/pull/68134.diff

1 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp (+6-4)
diff --git a/clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
index 98ae857b589fd64..7ad4b5fb7f043ab 100644
--- a/clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp
@@ -45,9 +45,10 @@ void CalleeNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
   if (FuncDecl->getBuiltinID() != 0)
     return;
 
-  // If the outermost namespace of the function is __llvm_libc, we're good.
+  // If the outermost namespace of the function starts with __llvm_libc, we're
+  // good.
   const auto *NS = dyn_cast<NamespaceDecl>(getOutermostNamespace(FuncDecl));
-  if (NS && NS->getName() == "__llvm_libc")
+  if (NS && NS->getName().starts_with("__llvm_libc"))
     return;
 
   const DeclarationName &Name = FuncDecl->getDeclName();
@@ -55,8 +56,9 @@ void CalleeNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
       IgnoredFunctions.contains(Name.getAsIdentifierInfo()->getName()))
     return;
 
-  diag(UsageSiteExpr->getBeginLoc(), "%0 must resolve to a function declared "
-                                     "within the '__llvm_libc' namespace")
+  diag(UsageSiteExpr->getBeginLoc(),
+       "%0 must resolve to a function declared "
+       "within the '__llvm_libc' namespace (use macro `LIBC_NAMESPACE`)")
       << FuncDecl;
 
   diag(FuncDecl->getLocation(), "resolves to this declaration",

Copy link
Contributor

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please update the release notes.

@EugeneZelenko
Copy link
Contributor

Shouldn't documentation be updated too?

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order of changes, test, documentation is missing

@gchatelet
Copy link
Contributor

It would be ideal to move the following lines to a shared header and use them in both implementations.

Copy link
Contributor Author

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed the comments with my latest commit

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From functional point of view looks fine, could be more strict but should still work.

@michaelrj-google michaelrj-google merged commit daca972 into llvm:main Oct 6, 2023
3 checks passed
metaflow added a commit that referenced this pull request Oct 9, 2023
for daca972 #68134 adds a namespace as
we are not using llvm::StringRef yet
@michaelrj-google michaelrj-google deleted the libcClangTidyFix branch November 1, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants