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