On Sun, Sep 20, 2015 at 1:44 PM, Felix Berger <f...@google.com> wrote:
> 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.
Yes, but why the requirement to check the triviality of the copy
twice? It seems to me that classHasTrivialCopyAndDestroy isn't really
a type trait that we want to expose the way you have. It should be a
private implementation detail of isExpensiveToCopy(), at which point
you can pull out the duplicated check for the triviality of the copy
constructor.
>
> ================
> 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?
I don't think it's an official one, but it resolves some type mismatch
issues with your code, which simplifies things.
>
> ================
> 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.
Oh, derp. I was expecting that to be on the copy operations, not on
the destructor. Sorry for the noise. :-)
~Aaron
>
> ================
> 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
