flx added inline comments.
================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
@@ +37,3 @@
+ Node.isTriviallyCopyableType(Finder->getASTContext()) ||
+ classHasTrivialCopyAndDestroy(Node)) {
+ return false;
----------------
aaron.ballman wrote:
> Why do you need classHasTrivialCopyAndDestroy() when you're already checking
> if it's a trivially copyable type?
We also want to catch types that have non-trivial destructors which would be
executed when the temporary copy goes out of scope.
================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
@@ +43,3 @@
+
+int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
+ const CXXConstructorDecl &ConstructorDecl,
----------------
aaron.ballman wrote:
> Should return unsigned, please.
Done. Is this an llvm convention?
================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
@@ +119,3 @@
+ }
+ diag(InitArg->getLocStart(), "value parameter can be moved to avoid copy.");
+}
----------------
alexfh wrote:
> 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)?
How would I tread in the IncludeOrder style (i.e. Google vs LLVM)? Should this
be a flag shared by all of ClangTidy or specific to this check?
================
Comment at: test/clang-tidy/misc-move-constructor-init.cpp:85
@@ +84,3 @@
+ Movable& operator =(const Movable&) = default;
+ ~Movable() {}
+};
----------------
aaron.ballman wrote:
> Why not = default?
We need to make the type non-trivially copyable by our definition above.
================
Comment at: test/clang-tidy/misc-move-constructor-init.cpp:113
@@ +112,3 @@
+
+struct NegativeParamTriviallyCopyable {
+ NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
----------------
aaron.ballman wrote:
> Should also have a test for scalars
Added.
http://reviews.llvm.org/D12839
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
