Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/docs/ReleaseNotes.rst | ||
---|---|---|
194 | Please keep alphabetical order (by check name) in this section. |
Requesting changes due to lack of support for base class initializes & got some concerns related lambdas used in initializers.
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp | ||
---|---|---|
406–413 | 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"))))) ); | |
472 | consider using here ->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit() | |
477–486 | 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 | |
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp | ||
1506 | Missing tests:
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) { } }; |
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp | ||
---|---|---|
406–413 | 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. | |
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp | ||
1506 | 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? |
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp | ||
---|---|---|
406–413 | yee, forgot type: hasDescendant(callExpr(equalsBoundNode("call-move")))) |
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp | ||
---|---|---|
1506 | For me: 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. |
Thanks, addressed some more review comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp | ||
---|---|---|
1506 | 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). |
clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp | ||
---|---|---|
1506 | Yes it could be. |
Overall LGTM, give it few days, maybe something will pop up.
I will try to run this change on my project and check if it finds any false-positives or real-issues.
Edit: Windows build failed due to clangd, try rebasing this to newer main branch
This is correct but consider this: