[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.
Paula Toth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 6 04:17:51 PDT 2020
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
More information about the cfe-commits
mailing list