[clang-tidy] Update llvmlibc-implementation-in-namespace to new rules

[gchatelet]

This is the implementation of step 3 of
https://discourse.llvm.org/t/rfc-customizable-namespace-to-allow-testing-the-libc-when-the-system-libc-is-also-llvms-libc/73079.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Changes

This is the implementation of step 3 of https://discourse.llvm.org/t/rfc-customizable-namespace-to-allow-testing-the-libc-when-the-system-libc-is-also-llvms-libc/73079.

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

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp (+13-8)
• (modified) clang-tools-extra/docs/clang-tidy/checks/llvmlibc/implementation-in-namespace.rst (+16-7)
• (modified) clang-tools-extra/test/clang-tidy/checkers/llvmlibc/implementation-in-namespace.cpp (+21-10)

diff --git a/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp b/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp index d05310f09ef773a..69a385f5be9807f 100644 --- a/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp @@ -14,7 +14,9 @@ using namespace clang::ast_matchers; namespace clang::tidy::llvm_libc { -const static StringRef RequiredNamespace = "__llvm_libc"; +const static StringRef RequiredNamespaceStart = "__llvm_libc"; +const static StringRef RequiredNamespaceMacroName = "LIBC_NAMESPACE"; + void ImplementationInNamespaceCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( decl(hasParent(translationUnitDecl()), unless(linkageSpecDecl())) @@ -29,16 +31,19 @@ void ImplementationInNamespaceCheck::check( if (!Result.SourceManager->isInMainFile(MatchedDecl->getLocation())) return; - if (const auto *NS = dyn_cast<NamespaceDecl>(MatchedDecl)) { - if (NS->getName() != RequiredNamespace) { - diag(NS->getLocation(), "'%0' needs to be the outermost namespace") - << RequiredNamespace; - } + if (auto *NS = dyn_cast<NamespaceDecl>(MatchedDecl)) { + if (!Result.SourceManager->isMacroBodyExpansion(NS->getLocation())) + diag(NS->getLocation(), + "the outermost namespace should be the '%0' macro") + << RequiredNamespaceMacroName; + else if (!NS->getName().starts_with(RequiredNamespaceStart)) + diag(NS->getLocation(), "the outermost namespace should start with '%0'") + << RequiredNamespaceStart; return; } diag(MatchedDecl->getLocation(), - "declaration must be declared within the '%0' namespace") - << RequiredNamespace; + "declaration must be declared within a namespace starting with '%0'") + << RequiredNamespaceStart; } } // 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 33d6dc8ff125c84..47ea2b866a93404 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,21 +8,30 @@ correct namespace. .. code-block:: c++ - // Correct: implementation inside the correct namespace. - namespace __llvm_libc { + // Implementation inside the LIBC_NAMESPACE namespace. + // Correct if: + // - LIBC_NAMESPACE is a macro + // - LIBC_NAMESPACE expansion starts with `__llvm_libc` + namespace LIBC_NAMESPACE { void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {} - // Namespaces within __llvm_libc namespace are allowed. - namespace inner{ + // Namespaces within LIBC_NAMESPACE namespace are allowed. + namespace inner { int localVar = 0; } // Functions with C linkage are allowed. - extern "C" void str_fuzz(){} + extern "C" void str_fuzz() {} } - // Incorrect: implementation not in a namespace. + // Incorrect: implementation not in the LIBC_NAMESPACE namespace. void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {} - // Incorrect: outer most namespace is not correct. + // Incorrect: outer most namespace is not the LIBC_NAMESPACE 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 { + 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 e75556a623b655c..16c5f9ca1067ec5 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,18 +3,18 @@ #define MACRO_A "defining macros outside namespace is valid" class ClassB; -// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within the '__llvm_libc' namespace +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within a namespace starting with '__llvm_libc' struct StructC {}; -// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration must be declared within the '__llvm_libc' namespace +// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration must be declared within a namespace starting with '__llvm_libc' char *VarD = MACRO_A; -// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within the '__llvm_libc' namespace +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within a namespace starting with '__llvm_libc' typedef int typeE; -// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: declaration must be declared within the '__llvm_libc' namespace +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: declaration must be declared within a namespace starting with '__llvm_libc' void funcF() {} -// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: declaration must be declared within the '__llvm_libc' namespace +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: declaration must be declared within a namespace starting with '__llvm_libc' namespace namespaceG { -// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: '__llvm_libc' needs to be the outermost namespace +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the outermost namespace should be the 'LIBC_NAMESPACE' macro namespace __llvm_libc{ namespace namespaceH { class ClassB; @@ -26,9 +26,20 @@ typedef int typeE; void funcF() {} } // namespace namespaceG -// Wrapped in correct namespace. -namespace __llvm_libc { -// Namespaces within __llvim_libc namespace allowed. +// Wrapped in macro namespace but with an incorrect name +#define LIBC_NAMESPACE custom_namespace +namespace LIBC_NAMESPACE { +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: the outermost namespace should start with '__llvm_libc' +namespace namespaceH { +class ClassB; +} // namespace namespaceH +} // namespace LIBC_NAMESPACE + + +// 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 { namespace namespaceI { class ClassB; } // namespace namespaceI @@ -37,4 +48,4 @@ char *VarD = MACRO_A; typedef int typeE; void funcF() {} extern "C" void extern_funcJ() {} -} // namespace __llvm_libc +} // namespace LIBC_NAMESPACE 

[carlosgalvezp]

Should this change be reflected in the Release Notes?

[gchatelet]

Should this change be reflected in the Release Notes?

Where would that be? clang-tools-extra/docs/ReleaseNotes.rst?
I can't find what the current release notes look like for head.

[PiotrZSL]

Where would that be? clang-tools-extra/docs/ReleaseNotes.rst? I can't find what the current release notes look like for head.

Yes, it's deployed here: https://clang.llvm.org/extra/ReleaseNotes.html

[PiotrZSL]

As a single small change to make hardcoded namespaces configurable looks fine.
I added some comments related to overall issues in this check.
Fell free to fix them or ignore them.

[gchatelet]

As a single small change to make hardcoded namespaces configurable looks fine. I added some comments related to overall issues in this check. Fell free to fix them or ignore them.

@PiotrZSL Thx for the review! I had to restructure the code a bit to accommodate for anonymous namespaces (they generate an implict UsingDirectiveDecl that was triggering the warning twice).

Let me know what you think.

[PiotrZSL]

LGTM