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