[PATCH] D11784: [PATCH] clang-tidy check for incorrect move constructor initializers

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

In http://reviews.llvm.org/D11784#224386, @alexfh wrote:

> A high level question: why is this check specific to base class initialization? Isn't the same issue possible when initializing members, for example? What would change if in your example `D` would have a member of type `B` instead of deriving from it?


Hmmm, I suppose this would apply equally to member initializers as well.

I was envisioning this as a strange dichotomy issue more than a performance issue. The caller goes to the trouble of move constructing, and you only half-move construct by calling the base class copy constructor which can potentially break class hierarchy invariants.

However, I think that's overthinking the situation and I should warn on member initialization that follows the same pattern.

> > One thing I am not certain of in this patch is how to test it. I have some rudimentary tests, but am unable to test the "note:" diagnostics from 

> 

> >  FileCheck (attempting to add any cause the "warning:" diagnostics to not be found).

> 

> 

> Can you give an example of what you do and what results do you get?


I put "CHECK: :[[@LINE+1]]:3: note: copy constructor being called" into the source file, and tests no longer pass because it cannot find the matching "warning: " diagnostic. If I then remove the warning diagnostic, the tests pass again. So it seems I can test one or the other, but not both. Specifically (with note and warning):

E:\llvm\2015>clang-tidy ..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp -checks=-*,misc-move-constructor-base -- -std=c++14 | FileCheck ..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp
2 warnings generated.
..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp:27:12: error: expected string not found in input
 // CHECK: :[[@LINE+1]]:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-base]

  ^

<stdin>:5:2: note: scanning from here
 B(const B&) {}
 ^
<stdin>:5:2: note: with expression "@LINE+1" equal to "28"
 B(const B&) {}
 ^
<stdin>:10:94: note: possible intended match here
E:\llvm\2015\..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp:70:16: warning: move constructor initializes base class by calling a copy constructor [misc-move-constructor-base]

               ^

with just note, no warning:

E:\llvm\2015>clang-tidy ..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp -checks=-*,misc-move-constructor-base -- -std=c++14 | FileCheck ..\llvm\tools\clang\tools\extra\test\clang-tidy\misc-move-constructor-base.cpp
2 warnings generated.

~Aaron

> 

> 

> > I suspect this is why clang-tidy.sh exists, but unfortunately, that means the tests will not be run on Windows (which is the platform I am 

> 

> >  developing on). Suggestions on how to improve the tests are welcome, but for now, I'm not testing the note diagnostics.

> 

> 

> Converting the script to python seems like the most universal approach. Should be trivial with the `sh` python package, but I'm not sure whether we can rely on it being installed.



http://reviews.llvm.org/D11784