[PATCH] D53882: [clang-tidy] Adding Zircon checker for std namespace

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp:71
+void NoStdNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<ValueDecl>("stdVar"))
+    diag(D->getBeginLoc(),
----------------
juliehockett wrote:
> JonasToth wrote:
> > Please create a `StringRef` for the diagnostic message and reuse that.
> > 
> > Did you consider merging all `Decl` classes if you just use `getNodeAs<Decl>("common_decl_name")`?
> Yes, but getting the location right posed an issue there. The `std` token is not always at `D->getLocation()`.
Ok, thats unfortunate but no problem :)


================
Comment at: clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp:52
+  Finder->addMatcher(
+      valueDecl(hasType(decl(hasDeclContext(namespaceDecl(isStdNamespace())))))
+          .bind("stdVar"),
----------------
are `FunctionDecl`, `RecordDecl` (maybe better `TagDecl`) covered already?


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:176
+
+  Warns when the `std` namespace is used, as its use is against Zircon's libc++
+  policy for the kernel.
----------------
s/its/it's/

Could `std` be considered code here? Not sure, but maybe using quotes is better?


================
Comment at: clang-tools-extra/test/clang-tidy/zircon-no-std-namespace.cpp:33
+int x = func();
+std::std_int y;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use of the 'std' namespace is not allowed in Zircon kernel code
----------------
juliehockett wrote:
> JonasToth wrote:
> > What about using `int64_t` and these typedefs. Are they forbidden, too?
> Yes, which is why that's tested. Is there an additional test case I'm missing regarding those?
Thanks for clarifying.

I think the usual problems (templates, macros) would need some test coverage.
Especially 

```
template <std::size_t NonTypeParm>
void MyFunc();
```

these cases are interesting to check.


https://reviews.llvm.org/D53882