[PATCH] D146288: clang-tidy: Detect use-after-move in CXXCtorInitializer

[Marco Falke via Phabricator via cfe-commits]

MarcoFalke added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:400
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   auto CallMoveMatcher =
+      callExpr(
----------------
PiotrZSL wrote:
> This is correct but consider this:
> 
> ```
> auto TryEmplaceMatcher = cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
> 
> auto CallMoveMatcher =
> callExpr(argumentCountIs(1), // because matching this will be faster than checking name.
>              callee(functionDecl(hasName("::std::move"))),
>              unless(inDecltypeOrTemplateArg()),
>              expr().bind("call-move"),
>              unless(hasParent(TryEmplaceMatcher)),
>              anyOf(hasAncestor(compoundStmt(hasParent(lambdaExpr().bind("containing-lambda")))),
>                         hasAncestor(functionDecl(anyOf(cxxConstructorDecl(hasAnyConstructorInitializer(
>                                                                                                                    withInitializer(expr(
>                                                                                                                           anyOf(equalsBounNode("call-move"), 
>                                                                                                                                      hasDescendant(equalsBounNode("call-move")))
>                                                                                                                      ).bind("containing-ctor-init-stmt")
>                                                                                                               ))
>                                                                                                              ).bind("containing-ctor"),
>                                                                              functionDecl().bind("containing-func")))))
>             );
> ```
>               
I get:

```
error: no matching function for call to object of type 'const internal::ArgumentAdaptingMatcherFunc<clang::ast_matchers::internal::HasDescendantMatcher>'
                    anyOf(equalsBoundNode("call-move"),hasDescendant(equalsBoundNode("call-move")))
                                                       ^~~~~~~~~~~~~
./llvm-project/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1491:3: note: candidate template ignored: could not match 'Matcher' against 'PolymorphicMatcher'
  operator()(const Matcher<T> &InnerMatcher) const {
  ^
./llvm-project/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1498:3: note: candidate template ignored: could not match 'MapAnyOfHelper' against 'PolymorphicMatcher'
  operator()(const MapAnyOfHelper<T...> &InnerMatcher) const {
  ^
1 error generated.
```


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1431
   std::string val_;
 };
----------------
PiotrZSL wrote:
> Missing tests:
> - Test with A derive from B, C, D, and argument is moved into C, but D initialization also uses it.
> - Test with lambda call inside init, and in lambda std::move of captured by reference constructor parameter, and lambda could be mutable, and lambda could be saved into member (or passed to base class), or called instantly (better both tests).
> - Test with lambda that takes argument into capture by move
> 
> In short:
> ```
> struct Obj {};
> struct D
> {
>     D(Obj b);
> };
> 
> struct C
> {
>      C(Obj b);
> };
> 
> struct B
> {
>      B(Obj& b);
> }
> 
> struct A : B, C, D
> {
>     A(Obj b)
>       : B(b), C(std::move(b)), D(b)
>     {
>     }
> };
> 
> and second:
> 
> struct Base
> {
>     Base(Obj b) : bb(std::move(b)) {}
>      template<typename T>
>      Base(T&& b) : bb(b()) {};
>    
>      Obj bb;
> };
> 
> struct Derived : Base, C
> {
>      Derived(Obj b) 
>            : Base([&] mutable { return std::move(b); }()), C(b)
>    {
>    }
> };
> 
> struct Derived2 : Base, C
> {
>      Derived2(Obj b) 
>            : Base([&] mutable { return std::move(b); }), C(b)
>    {
>    }
> };
> 
> struct Derived3 : Base, C
> {
>      Derived3(Obj b) 
>            : Base([c = std::move(b)] mutable { return std::move(c); }), C(b)
>    {
>    }
> };
> ```
I added the test `Derived2`, however, I wonder if it should throw a warning. In the general case, I don't think we can know whether `Base(Call&&call)` will actually call `call` and execute the `std::move`, no?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146288/new/

https://reviews.llvm.org/D146288