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