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

[Aaron Ballman via cfe-commits]

On Sun, Sep 20, 2015 at 1:44 PM, Felix Berger <flx at 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
>
>
>