[PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Oops, forgot to hit "Submit" yesterday. Sorry for the delay.


================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:28
@@ +27,3 @@
+
+AST_MATCHER(QualType, isExpensiveToCopy) {
+  // We can't reason about dependent types. Ignore them.
----------------
These helper functions can (and will) be useful for other checks as well. Let's move them to `clang-tidy/utils/ `. We can add `TypeTraits.h` for functions like `classHasTrivialCopyAndDestroy`, `isExpensiveTypeToCopy`, etc. And a separate header (`Matchers.h`, for example) for the matchers that are not suitable for the ASTMatchers.h.

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:100
@@ -36,1 +99,3 @@
 void MoveConstructorInitCheck::check(const MatchFinder::MatchResult &Result) {
+  if (Result.Nodes.getNodeAs<CXXCtorInitializer>("move-init") != nullptr) {
+    handleMoveConstructor(Result);
----------------
nit: It's common to omit braces around single-line statements in LLVM/Clang code.

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
@@ +119,3 @@
+  }
+  diag(InitArg->getLocStart(), "value parameter can be moved to avoid copy.");
+}
----------------
aaron.ballman wrote:
> Perhaps: "argument can be moved to avoid a copy" instead?
nit: Please remove the trailing period.

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
@@ +119,3 @@
+  }
+  diag(InitArg->getLocStart(), "value parameter can be moved to avoid copy.");
+}
----------------
alexfh wrote:
> aaron.ballman wrote:
> > Perhaps: "argument can be moved to avoid a copy" instead?
> nit: Please remove the trailing period.
Does anything stop us from suggesting fixes here (inserting "std::move()" around the argument and #include <utility>, if it's no there yet)?

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.h:21
@@ -20,1 +20,3 @@
 /// move constructor.
+/// It also flags constructor arguments that are passed by value, have a
+/// non-deleted move-constructor and are assigned to a class field by copy
----------------
I suggest updating the corresponding .rst file instead and adding the URL of the documentation in the comment along with a short summary of what the check does.

For example:
```
/// Flag missing move opportunities in constructor initializer lists.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/misc-move-constructor-init.html
```


http://reviews.llvm.org/D12839