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

[Paula Toth via Phabricator via libc-commits]

PaulkaToast 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"),
----------------
aaron.ballman wrote:
> This skips linkage spec declarations, but are there other declarations that should be similarly skipped? For instance `static_assert` declarations?
I believe that linkage is the only exception needed, static_asserts and all other declarations should also be within the namespace. 


================
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) {
----------------
aaron.ballman wrote:
> Instead of doing an `isa<>` followed by a `cast<>`, the more common pattern is to do:
> ```
> if (const auto *NS = dyn_cast<NamespaceDecl>(MatchedDecl)) {
> ```
Ah thank you this looks much nicer!


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


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h:35
+private:
+  std::string RequiredNamespace;
+};
----------------
aaron.ballman wrote:
> 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)?
There will only be this one 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`.
----------------
aaron.ballman wrote:
> Given that this check is specific to llvm-libc, why is the option needed at all?
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818