[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Thank you for the explanation about what is driving this and the documentation. I don't think google-* is the correct module for this as it's too general of an umbrella. I have a slight preference for zircon-* because this appears to be specific to that particular project.



================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:18
+
+AST_MATCHER(ParmVarDecl, hasDefaultArgument) { return Node.hasDefaultArg(); }
+
----------------
I think this should be put into ASTMatchers.h as a separate patch (feel free to list me as a reviewer). See D39940 for an example of how to do that, but the only non-obvious task is running clang\docs\tools\dump_ast_matchers.py to generate the HTML from ASTMatchers.h.


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:22
+  // Calling a function which uses default arguments is disallowed.
+  Finder->addMatcher(cxxDefaultArgExpr().bind("stmt"), this);
+  // Declaring default parameters is disallowed.
----------------
Isn't this case already covered by the other matcher? Or do you have system header files which have default arguments and those functions are disallowed by the style guide, but cannot be removed from the system header?


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:28
+void DefaultArgumentsCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const CXXDefaultArgExpr *S =
+          Result.Nodes.getNodeAs<CXXDefaultArgExpr>("stmt")) {
----------------
You can use `const auto *` here instead of spelling the type out twice.


================
Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:36
+    diag(D->getLocStart(),
+         "declaring functions which use default arguments is disallowed");
+  }
----------------
Would it make sense to add a fix-it that removes the intializer for the default argument?


================
Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:8
+
+Example: The declaration:
+
----------------
Eugene.Zelenko wrote:
> I briefly look on other checks documentation, so will be good idea to use just //Example:// or //For example, the declaration:// . But will be good idea to hear opinion of native English speaker.
I'd go with: `For example, the declaration:`


================
Comment at: docs/clang-tidy/index.rst:672
   will run clang-format over changed lines.
-
----------------
Spurious newline removal?


https://reviews.llvm.org/D40108