[PATCH] D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

A few nits. Otherwise looks good, if Sam has no objections.


================
Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:34
@@ -29,1 +33,3 @@
 
+template <typename S> bool isSetDifferenceEmpty(const S &S1, const S &S2) {
+  for (const auto &E : S1)
----------------
+1 to `isSubset`. And please add a comment explaining what should be a subset of what for this function to return `true`.

================
Comment at: clang-tidy/utils/Matchers.h:50
@@ +49,3 @@
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
+  return ast_matchers::referenceType(ast_matchers::pointee(
+      ast_matchers::qualType(ast_matchers::isConstQualified())));
----------------
How about `using ast_matchers;` in this function to make the statement easier to read?

================
Comment at: clang-tidy/utils/TypeTraits.cpp:129
@@ +128,3 @@
+  auto *Record = Type->getAsCXXRecordDecl();
+  if (!Record || !Record->hasDefinition())
+    return false;
----------------
Maybe simplify this a bit?

  return Record && Record->hasDefinition() && Record->hasNonTrivialMoveAssignment();

================
Comment at: clang-tidy/utils/TypeTraits.cpp:136
@@ +135,3 @@
+  auto *Record = Type->getAsCXXRecordDecl();
+  if (!Record || !Record->hasDefinition())
+    return false;
----------------
ditto

================
Comment at: clang-tidy/utils/TypeTraits.h:32
@@ -31,1 +31,3 @@
 
+// Returns true if Type has a non-trivial move constructor.
+bool hasNonTrivialMoveConstructor(QualType Type);
----------------
Please use doxygen comments (`///`) and enclose inline code snippets in backquotes. The other function comments in this file should also be doxygen, I've fixed them in r272994.


http://reviews.llvm.org/D20277