aaron.ballman added inline comments.
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:53
@@ +52,3 @@
+/// matches the given matcher.
+AST_MATCHER_P(QualType, ignoringRefsAndConsts,
+ ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
----------------
Prazek wrote:
> This one is usefull AF. Can you put into Traversal AST Matchers?
>
>
> {meme, src=brilliant}
>
>
I don't think this is a good candidate for a public AST matcher -- it does too
many disparate things.
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:64
@@ +63,3 @@
+/// \brief Matches ParmVarDecls which have default arguments.
+AST_MATCHER(ParmVarDecl, hasDefaultArgument) { return Node.hasDefaultArg(); }
+
----------------
With proper documentation and tests, this would be a good candidate to move
into ASTMatchers.h though. However, it should be called `hasDefaultArg()`
instead.
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:69
@@ +68,3 @@
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
+ hasOneActiveArgument) {
+ return anyOf(parameterCountIs(1),
----------------
Arguments are what the caller provides. Parameters are what the function
accepts. Also "active" is a bit of a strange term for here, so I think this
should be renamed to something like `hasOneNonDefaultedParam()` or something
similar.
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:77
@@ +76,3 @@
+/// matcher.
+AST_MATCHER_P(TemplateTypeParmDecl, hasTemplateType,
+ ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
----------------
I think we should make `hasType()` work for `TemplateTypeParmDecl` instead of
doing it this way (as part of the public AST matcher interface), assuming it
doesn't currently work.
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:86
@@ +85,3 @@
+/// to move-construct from any type.
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<CXXConstructorDecl>,
+ moveConstructorFromAnyType) {
----------------
I think this might actually be better expressed as a local variable in
`registerMatchers()` instead of a matcher like this.
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:88
@@ +87,3 @@
+ moveConstructorFromAnyType) {
+ // Potentially danger: this matcher binds a name, with probably
+ // mean that you cant use it twice in your check!
----------------
s/danger/dangerous
s/with/which
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:89
@@ +88,3 @@
+ // Potentially danger: this matcher binds a name, with probably
+ // mean that you cant use it twice in your check!
+ const char TemplateArgument[] = "templateArgument";
----------------
s/mean/means
s/cant/can't
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:111-112
@@ +110,4 @@
+
+/// \brief Matches to qual types which has constructors from type that matches
+/// the given matcher.
+AST_MATCHER_FUNCTION_P(ast_matchers::internal::Matcher<QualType>,
----------------
Can you fix the grammatical issues with the comment? I'm not quite certain what
it should say, but I think it's something like:
Matches a QualType whose declaration has a converting constructor accepting an
argument of the type from the given inner matcher.
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.cpp:130-131
@@ +129,4 @@
+
+ // Matches to type with after ignoring refs and consts is the same as
+ // "constructedType"
+ auto HasTypeSameAsConstructed = hasType(hasCanonicalType(
----------------
Also ungrammatical (and missing a period at the end of the sentence).
================
Comment at: clang-tidy/performance/ReturnValueCopyCheck.h:20
@@ +19,3 @@
+
+/// This check finds places where we are returning object of a different type
than
+/// the function return type. In such places, we should use std::move,
otherwise
----------------
an object of a different type.
http://reviews.llvm.org/D21303
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
