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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:40
@@ +39,3 @@
+  for (const auto *Ctor : CopyCtor->getParent()->ctors()) {
+    if (Ctor->isMoveConstructor() &&
+        Ctor->getAccess() <= AS_protected &&
----------------
alexfh wrote:
> clang-format?
I thought the current formatting was an improvement over what clang-format produces (for those of us with debuggers that aren't as good at subexpression highlighting). I'm fine either way, though.

================
Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:46
@@ +45,3 @@
+      //
+      // FIXME: Determine whether the move constructor is a viable candidate
+      // for the ctor-initializer, perhaps provide a fixit that suggests
----------------
alexfh wrote:
> This seems to be rather important to do from the beginning. Otherwise the check may be too noisy. BTW, did you run it over LLVM and Clang sources? Would be useful for some smoke testing.
In order to do that, I would need access to more parts of Sema. The check, as it currently stands, is fairly reasonable from what I can tell. The false positive rate appears to be low. I ran it over Clang and LLVM and it did point out one debatably-true-positive (which we've since resolved) with no false-positives. In testing other code bases, the diagnostic was not chatty, but perhaps they did not make heavy use of move semantics.

================
Comment at: test/clang-tidy/misc-move-constructor-init.cpp:64
@@ +63,3 @@
+struct J : I {
+  // CHECK-NOT: warning:
+  J(J &&RHS) : I(RHS) {} // ok
----------------
alexfh wrote:
> I'd suggest using FileCheck -implicit-check-not='{{warning|error}}:' instead of stuffing the code with `// CHECK-NOT: warning:`. It will make the test more consistent with the other tests that use the clang_tidy_test.sh script.
Can do (though I am explicitly not using clang_tidy_test.sh because I am working on Windows and all those tests are currently disabled due to REQUIRES: shell :-()


http://reviews.llvm.org/D11784