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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
