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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1431
   std::string val_;
 };
----------------
MarcoFalke wrote:
> PiotrZSL wrote:
> > MarcoFalke wrote:
> > > 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?
> > For me:
> > Derived - should throw a warning - same type used before C(b) (but questionable)
> > Derived 2 -> also should throw warning (but questionable)
> > Derived 3 -> also should throw warning
> > 
> > Note that Derived & Derived 2 could be considered different issue, like use after free (because we take reference to argument), it's safe to assume that it would be called anyway, because later that argument reference could become invalid. But it could be also fine to not throw issue. For is better to throw & nolint than ignore and crash.
> > But thats your call.
> Ok, maybe it can be done in a follow-up? This should be a separate and pre-existing issue on `main` already (see also my other added test case about lambdas).
Yes it could be.


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

https://reviews.llvm.org/D146288