[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:29
+AST_MATCHER(NamespaceAliasDecl, isAliasedToStd) {
+  if (const auto *AN = Node.getAliasedNamespace()) {
+    // If this aliases to an actual namespace, check if its std. If it doesn't,
----------------
please do not use `auto` here as the type is not clear, same below (line 32 right now)


================
Comment at: clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.cpp:31
+    // If this aliases to an actual namespace, check if its std. If it doesn't,
+    // it aliases to another alias and thus shouldn't be flagged.
+    if (const auto *N = dyn_cast<NamespaceDecl>(AN))
----------------
The second sentence sounds a bit weird. I think you can even ellide it completely, as the first sentence does clarify quite well.


================
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(),
----------------
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")`?


================
Comment at: clang-tools-extra/clang-tidy/zircon/NoStdNamespaceCheck.h:1
+//===--- NoStdNamespace.h - clang-tidy---------------------------*- C++ -*-===//
+//
----------------
There was a bug in the template, please add a blank after `clang-tidy` but keep the total length.


================
Comment at: clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp:28
         "zircon-temporary-objects");
+    CheckFactories.registerCheck<NoStdNamespaceCheck>(
+        "zircon-no-std-namespace");
----------------
please keep lexigraphical ordering.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/zircon-no-std-namespace.rst:1
+.. title:: clang-tidy - zircon-no-std-namespace
+
----------------
Could you please clarify what exactly is forbidden?

__Using__ stuff from `std` like `std::string` in normal "user" code or __opening__ `std` to add stuff?

If the first thing is the case why are the variables flagged but not the functions? And adding things to namespace `std` is AFAIK UB with the exception of template-function specializations, so maybe that would be worth a general check?


================
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
----------------
What about using `int64_t` and these typedefs. Are they forbidden, too?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53882