[clang-tidy] Add check llvmlibc-implementation-in-namespace.

[PaulkaToast]

Summary

This check makes sure all llvm-libc implementations falls within the __llvm_libc namespace.

The check is quite general, lots of projects do this by conversion to avoid pollution. Not sure if there is a desire to put this in a different module?

[Eugene.Zelenko]

Please also add isLanguageVersionSupported(), because namespaces make sense only in C++.

[sivachandra]

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

31

For completeness, can you include a nested namespace and a function declaration here?

[njames93]

If you want to make it a general check, you should consider making the default module options set the correct namespace
RequiredNamespace

ClangTidyOptions getModuleOptions() override {
  ClangTidyOptions Options;
  Options.CheckOptions["llvmlibc-implementation-in-namespace.RequiredNamespace"] = "__llvm_libc";
  return Options;
}

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

7

Small nit : Not a fan of the diagnostic saying CXX Record. Maybe a pain but getDeclKindName isn't the best to expose to users.

[PaulkaToast]

Updated to handle nested namespaces, exclude C linkages functions, and made check language specific. (:

[PaulkaToast]

In D76818#1943781, @njames93 wrote:

If you want to make it a general check, you should consider making the default module options set the correct namespace
RequiredNamespace

Changed the check a bit so I'm not sure if pulling it out is still something desired, if so what module do you recommend this should live under?

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

7

Removed getDeclKindName as recommended.

[PaulkaToast]

Friendly Ping @njames93 (:
Any other changes needed?

[sivachandra]

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

16

Can you add __llvm_libc as a nested namespace here.

[njames93]

This check needs a store Options override method as it reading options from a configuration. Check other checks to see how this is done.

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h

36

This can be made const

[aaron.ballman]

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp

22

This skips linkage spec declarations, but are there other declarations that should be similarly skipped? For instance static_assert declarations?

34–35

Instead of doing an isa<> followed by a cast<>, the more common pattern is to do:

if (const auto *NS = dyn_cast<NamespaceDecl>(MatchedDecl)) {

37

clang-tidy diagnostics don't have punctuation, so you should drop the full stop.

43

They also aren't grammatically correct sentences, so the capital P and period should both go. While this definitely gets points for politeness, I think a more typical diagnostic might be: declaration must be declared within the '%0' namespace

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h

36

Will there only ever be a single namespace? Or should this be a list (for instance, a main namespace and a details namespace)?

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst

7

Checks that all declarations in the llvm-libc implementation are within the correct namespace.

33–36

Given that this check is specific to llvm-libc, why is the option needed at all?

[PaulkaToast]

Addressed njames's and aaron's comments. (:

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp

22

I believe that linkage is the only exception needed, static_asserts and all other declarations should also be within the namespace.

34–35

Ah thank you this looks much nicer!

43

ah, alright, good call. (:

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h

36

There will only be this one namespace.

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst

33–36

I was concerned that maybe there would be a desire to make the check generalized, however it does seem quite specific so I will take your advice.

[aaron.ballman]

LGTM!

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp

22

Excellent, thank you for verifying!

clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyLLVMLibcModule
+  ImplementationInNamespaceCheck.cpp
   LLVMLibcTidyModule.cpp
   RestrictSystemLibcHeadersCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyPortabilityModule
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h

+//===--- ImplementationInNamespaceCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVMLIBC_IMPLEMENTATIONINNAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVMLIBC_IMPLEMENTATIONINNAMESPACECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace llvm_libc {
+
+/// Checks all llvm-libc implementation is within the correct namespace.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvmlibc-implementation-in-namespace.html
+class ImplementationInNamespaceCheck : public ClangTidyCheck {
+public:
+  ImplementationInNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        RequiredNamespace(Options.get("RequiredNamespace", "__llvm_libc")) {}
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::string RequiredNamespace;
+};
+
+} // namespace llvm_libc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVMLIBC_IMPLEMENTATIONINNAMESPACECHECK_H

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp

+//===--- ImplementationInNamespaceCheck.cpp - clang-tidy ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ImplementationInNamespaceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace llvm_libc {
+
+void ImplementationInNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      decl(hasParent(translationUnitDecl()), unless(linkageSpecDecl()))
+          .bind("child_of_translation_unit"),
+      this);
+}
+
+void ImplementationInNamespaceCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl =
+      Result.Nodes.getNodeAs<Decl>("child_of_translation_unit");
+  if (!Result.SourceManager->isInMainFile(MatchedDecl->getLocation()))
+    return;
+
+  if (isa<NamespaceDecl>(MatchedDecl)) {
+    const auto *NS = cast<NamespaceDecl>(MatchedDecl);
+    if (NS->getName() != RequiredNamespace) {
+      diag(NS->getLocation(), "'%0' needs to be the outermost namespace.")
+          << RequiredNamespace;
+    }
+    return;
+  }
+  diag(MatchedDecl->getLocation(),
+       "Please wrap implentation in '%0' namespace.")
+      << RequiredNamespace;
+  return;
+}
+
+} // namespace llvm_libc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp

 //===--- LLVMLibcTidyModule.cpp - clang-tidy ------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "ImplementationInNamespaceCheck.h"
 #include "RestrictSystemLibcHeadersCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace llvm_libc {
 
 class LLVMLibcModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ImplementationInNamespaceCheck>(
+        "llvmlibc-implementation-in-namespace");
     CheckFactories.registerCheck<RestrictSystemLibcHeadersCheck>(
         "llvmlibc-restrict-system-libc-headers");
   }
 };
 
 // Register the LLVMLibcTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<LLVMLibcModule>
     X("llvmlibc-module", "Adds LLVM libc standards checks.");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-suspicious-include
   them.
 
 - New :doc:`cert-oop57-cpp
   <clang-tidy/checks/cert-oop57-cpp>` check.
 
   Flags use of the `C` standard library functions ``memset``, ``memcpy`` and
   ``memcmp`` and similar derivatives on non-trivial types.
 
+- New :doc:`llvmlibc-implementation-in-namespace
+  <clang-tidy/checks/llvmlibc-implementation-in-namespace>` check.
+
+  Checks all llvm-libc implementation is within the correct namespace.
+
 - New :doc:`llvmlibc-restrict-system-libc-headers
   <clang-tidy/checks/llvmlibc-restrict-system-libc-headers>` check.
 
   Finds includes of system libc headers not provided by the compiler within
   llvm-libc implementations.
 
 - New :doc:`objc-dealloc-in-category
   <clang-tidy/checks/objc-dealloc-in-category>` check.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `hicpp-signed-bitwise <hicpp-signed-bitwise.html>`_,
    `linuxkernel-must-use-errs <linuxkernel-must-use-errs.html>`_,
    `llvm-header-guard <llvm-header-guard.html>`_,
    `llvm-include-order <llvm-include-order.html>`_, "Yes"
    `llvm-namespace-comment <llvm-namespace-comment.html>`_,
    `llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm-prefer-isa-or-dyn-cast-in-conditionals.html>`_, "Yes"
    `llvm-prefer-register-over-unsigned <llvm-prefer-register-over-unsigned.html>`_, "Yes"
    `llvm-twine-local <llvm-twine-local.html>`_, "Yes"
+   `llvmlibc-implementation-in-namespace <llvmlibc-implementation-in-namespace.html>`_,
    `llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_, "Yes"
    `misc-definitions-in-headers <misc-definitions-in-headers.html>`_, "Yes"
    `misc-misplaced-const <misc-misplaced-const.html>`_,
    `misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
    `misc-no-recursion <misc-no-recursion.html>`_,
    `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
    `misc-non-private-member-variables-in-classes <misc-non-private-member-variables-in-classes.html>`_,
    `misc-redundant-expression <misc-redundant-expression.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst

+.. title:: clang-tidy - llvmlibc-implementation-in-namespace
+
+llvmlibc-implementation-in-namespace
+====================================
+
+Checks all llvm-libc implementation is within the correct namespace.
+
+.. code-block:: c++
+
+    // Correct: implementation inside the correct namespace.
+    namespace __llvm_libc {
+        void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+        // Namespaces within __llvm_libc namespace are allowed.
+        namespace inner{
+            int localVar = 0;
+        }
+        // Functions with C linkage are allowed.
+        extern "C" void str_fuzz(){}
+    }
+
+    // Incorrect: implementation not in a namespace.
+    void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+
+    // Incorrect: outer most namespace is not correct.
+    namespace something_else {
+        void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+    }
+
+Options
+-------
+
+.. option:: RequiredNamespace
+
+    The namespace that llvm-libc implementations must be wrapped in. The default
+    is `__llvm_libc`.

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

+// RUN: %check_clang_tidy %s llvmlibc-implementation-in-namespace %t
+
+#define MACRO_A "defining macros outside namespace is valid"
+
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Please wrap implentation in '__llvm_libc' namespace.
+struct StructC {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: Please wrap implentation in '__llvm_libc' namespace.
+char *VarD = MACRO_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Please wrap implentation in '__llvm_libc' namespace.
+typedef int typeE;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Please wrap implentation in '__llvm_libc' namespace.
+void funcF() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Please wrap implentation in '__llvm_libc' namespace.
+
+namespace namespaceG {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: '__llvm_libc' needs to be the outermost namespace.
+namespace namespaceH {
+class ClassB;
+} // namespace namespaceH
+struct StructC {};
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+} // namespace namespaceG
+
+// Wrapped in correct namespace.
+namespace __llvm_libc {
+// Namespaces within __llvim_libc namespace allowed.
+namespace namespaceI {
+class ClassB;
+} // namespace namespaceI
+struct StructC {};
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+extern "C" void extern_funcJ() {}
+} // namespace __llvm_libc