PiotrZSL requested changes to this revision. PiotrZSL added a comment. This revision now requires changes to proceed.
Requesting changes due to lack of support for base class initializes & got some concerns related lambdas used in initializers. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:400 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { auto CallMoveMatcher = + callExpr( ---------------- 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"))))) ); ``` ================ Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:474 + for (CXXCtorInitializer *Init : ContainingCtor->inits()) { + if (Init->getInit() == ContainingCtorInitStmt) { + ContainingCtorField = Init->getMember(); ---------------- consider using here `->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit()` ================ Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:479-488 + if (ContainingCtorField) { + // Collect the constructor initializer expressions. + for (CXXCtorInitializer *Init : ContainingCtor->inits()) { + if (Init->getMember() && Init->getMember()->getFieldIndex() >= + ContainingCtorField->getFieldIndex()) { + if (Init->getInit()) + CodeBlocks.push_back(Init->getInit()); ---------------- This won't work for std::move used to initialize base class. Assuming that Inits are in specific order, then you just should include all init's after one found, or use things like `SourceManager::isBeforeInTranslationUnit`, but still probably using something like: ``` bool PastMove = false; for (CXXCtorInitializer *Init : ContainingCtor->inits()) { if (!PastMove && Init->getInit()->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit()) { PastMove = true; } if (PastMove && Init->getInit()) { CodeBlocks.push_back(Init->getInit()); } ``` would be better, as it should work for both field inits and base constructor calls ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1431 std::string val_; }; ---------------- 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) { } }; ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146288/new/ https://reviews.llvm.org/D146288 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits