[libc-commits] [PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Aaron Ballman via Phabricator via libc-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:21
+  Finder->addMatcher(
+      decl(hasParent(translationUnitDecl()), unless(linkageSpecDecl()))
+          .bind("child_of_translation_unit"),
----------------
This skips linkage spec declarations, but are there other declarations that should be similarly skipped? For instance `static_assert` declarations?


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:33-34
+
+  if (isa<NamespaceDecl>(MatchedDecl)) {
+    const auto *NS = cast<NamespaceDecl>(MatchedDecl);
+    if (NS->getName() != RequiredNamespace) {
----------------
Instead of doing an `isa<>` followed by a `cast<>`, the more common pattern is to do:
```
if (const auto *NS = dyn_cast<NamespaceDecl>(MatchedDecl)) {
```


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:36
+    if (NS->getName() != RequiredNamespace) {
+      diag(NS->getLocation(), "'%0' needs to be the outermost namespace.")
+          << RequiredNamespace;
----------------
clang-tidy diagnostics don't have punctuation, so you should drop the full stop.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:42
+  diag(MatchedDecl->getLocation(),
+       "Please wrap implentation in '%0' namespace.")
+      << RequiredNamespace;
----------------
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`


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h:35
+private:
+  std::string RequiredNamespace;
+};
----------------
njames93 wrote:
> This can be made const
Will there only ever be a single namespace? Or should this be a list (for instance, a main namespace and a details namespace)?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst:6
+
+Checks all llvm-libc implementation is within the correct namespace.
+
----------------
Checks that all declarations in the llvm-libc implementation are within the correct namespace.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst:32-35
+.. option:: RequiredNamespace
+
+    The namespace that llvm-libc implementations must be wrapped in. The default
+    is `__llvm_libc`.
----------------
Given that this check is specific to llvm-libc, why is the option needed at all?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76818/new/

https://reviews.llvm.org/D76818