[PATCH] D21303: [clang-tidy] Adds performance-returning-type check.

[Aaron Ballman via cfe-commits]

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