From: Stephen Kelly Date: Mon, 28 Dec 2020 01:47:25 +0000 (+0000) Subject: [clang-tidy] Simplify unused RAII check X-Git-Tag: llvmorg-14-init~13590 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=7b6fc9a1055a7eae07645fb7d3085dc89a8d54ab;p=platform%2Fupstream%2Fllvm.git [clang-tidy] Simplify unused RAII check Fix handling of default construction where the constructor has a default arg. Differential Revision: https://reviews.llvm.org/D97142 --- diff --git a/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp index 3a52180..b87bb9e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp @@ -25,25 +25,37 @@ AST_MATCHER(CXXRecordDecl, hasNonTrivialDestructor) { void UnusedRaiiCheck::registerMatchers(MatchFinder *Finder) { // Look for temporaries that are constructed in-place and immediately - // destroyed. Look for temporaries created by a functional cast but not for - // those returned from a call. - auto BindTemp = cxxBindTemporaryExpr( - unless(has(ignoringParenImpCasts(callExpr()))), - unless(has(ignoringParenImpCasts(objcMessageExpr())))) - .bind("temp"); + // destroyed. Finder->addMatcher( - traverse(TK_AsIs, - exprWithCleanups( - unless(isInTemplateInstantiation()), - hasParent(compoundStmt().bind("compound")), - hasType(cxxRecordDecl(hasNonTrivialDestructor())), - anyOf(has(ignoringParenImpCasts(BindTemp)), - has(ignoringParenImpCasts(cxxFunctionalCastExpr( - has(ignoringParenImpCasts(BindTemp))))))) - .bind("expr")), + mapAnyOf(cxxConstructExpr, cxxUnresolvedConstructExpr) + .with(hasParent(compoundStmt().bind("compound")), + anyOf(hasType(cxxRecordDecl(hasNonTrivialDestructor())), + hasType(templateSpecializationType( + hasDeclaration(classTemplateDecl(has( + cxxRecordDecl(hasNonTrivialDestructor())))))))) + .bind("expr"), this); } +template +void reportDiagnostic(DiagnosticBuilder D, const T *Node, SourceRange SR, + bool DefaultConstruction) { + const char *Replacement = " give_me_a_name"; + + // If this is a default ctor we have to remove the parens or we'll introduce a + // most vexing parse. + if (DefaultConstruction) { + D << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(SR), + Replacement); + return; + } + + // Otherwise just suggest adding a name. To find the place to insert the name + // find the first TypeLoc in the children of E, which always points to the + // written type. + D << FixItHint::CreateInsertion(SR.getBegin(), Replacement); +} + void UnusedRaiiCheck::check(const MatchFinder::MatchResult &Result) { const auto *E = Result.Nodes.getNodeAs("expr"); @@ -55,35 +67,32 @@ void UnusedRaiiCheck::check(const MatchFinder::MatchResult &Result) { // Don't emit a warning for the last statement in the surrounding compound // statement. const auto *CS = Result.Nodes.getNodeAs("compound"); - if (E == CS->body_back()) + const auto *LastExpr = dyn_cast(CS->body_back()); + + if (LastExpr && E == LastExpr->IgnoreUnlessSpelledInSource()) return; // Emit a warning. auto D = diag(E->getBeginLoc(), "object destroyed immediately after " "creation; did you mean to name the object?"); - const char *Replacement = " give_me_a_name"; - // If this is a default ctor we have to remove the parens or we'll introduce a - // most vexing parse. - const auto *BTE = Result.Nodes.getNodeAs("temp"); - if (const auto *TOE = dyn_cast(BTE->getSubExpr())) - if (TOE->getNumArgs() == 0) { - D << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(TOE->getParenOrBraceRange()), - Replacement); - return; + if (const auto *Node = dyn_cast(E)) + reportDiagnostic(D, Node, Node->getParenOrBraceRange(), + Node->getNumArgs() == 0 || + isa(Node->getArg(0))); + if (const auto *Node = dyn_cast(E)) { + auto SR = SourceRange(Node->getLParenLoc(), Node->getRParenLoc()); + auto DefaultConstruction = Node->getNumArgs() == 0; + if (!DefaultConstruction) { + auto FirstArg = Node->getArg(0); + DefaultConstruction = isa(FirstArg); + if (auto ILE = dyn_cast(FirstArg)) { + DefaultConstruction = ILE->getNumInits() == 0; + SR = SourceRange(ILE->getLBraceLoc(), ILE->getRBraceLoc()); + } } - - // Otherwise just suggest adding a name. To find the place to insert the name - // find the first TypeLoc in the children of E, which always points to the - // written type. - auto Matches = - match(expr(hasDescendant(typeLoc().bind("t"))), *E, *Result.Context); - if (const auto *TL = selectFirst("t", Matches)) - D << FixItHint::CreateInsertion( - Lexer::getLocForEndOfToken(TL->getEndLoc(), 0, *Result.SourceManager, - getLangOpts()), - Replacement); + reportDiagnostic(D, Node, SR, DefaultConstruction); + } } } // namespace bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.h index 3608365..f4a0de0 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.h @@ -28,6 +28,9 @@ public: } void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + llvm::Optional getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } }; } // namespace bugprone diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-raii.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-raii.cpp index 3428d45..9464367 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-raii.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-raii.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s bugprone-unused-raii %t +// RUN: %check_clang_tidy %s bugprone-unused-raii %t -- -- -fno-delayed-template-parsing struct Foo { Foo(); @@ -46,6 +46,42 @@ void neverInstantiated() { T(); } +struct CtorDefaultArg { + CtorDefaultArg(int i = 0); + ~CtorDefaultArg(); +}; + +template +struct TCtorDefaultArg { + TCtorDefaultArg(T i = 0); + ~TCtorDefaultArg(); +}; + +struct CtorTwoDefaultArg { + CtorTwoDefaultArg(int i = 0, bool b = false); + ~CtorTwoDefaultArg(); +}; + +template +void templatetest() { + TCtorDefaultArg(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? + // CHECK-FIXES: TCtorDefaultArg give_me_a_name; + TCtorDefaultArg{}; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? + // CHECK-FIXES: TCtorDefaultArg give_me_a_name; + + TCtorDefaultArg(T{}); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? + // CHECK-FIXES: TCtorDefaultArg give_me_a_name(T{}); + TCtorDefaultArg{T{}}; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? + // CHECK-FIXES: TCtorDefaultArg give_me_a_name{T{}}; + + int i = 0; + (void)i; +} + void test() { Foo(42); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? @@ -71,6 +107,22 @@ void test() { // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? // CHECK-FIXES: FooBar give_me_a_name; + CtorDefaultArg(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? + // CHECK-FIXES: CtorDefaultArg give_me_a_name; + + CtorTwoDefaultArg(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? + // CHECK-FIXES: CtorTwoDefaultArg give_me_a_name; + + TCtorDefaultArg(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? + // CHECK-FIXES: TCtorDefaultArg give_me_a_name; + + TCtorDefaultArg{}; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object? + // CHECK-FIXES: TCtorDefaultArg give_me_a_name; + templ(); templ();