On Tue, Jul 22, 2014 at 5:30 AM, Benjamin Kramer <[email protected]> wrote:
> Author: d0k > Date: Tue Jul 22 07:30:35 2014 > New Revision: 213647 > > URL: http://llvm.org/viewvc/llvm-project?rev=213647&view=rev > Log: > [clang-tidy] Add a check for RAII temporaries. > > Summary: > This tries to find code similar that immediately destroys > an object that looks like it's trying to follow RAII. > { > scoped_lock(&global_mutex); > critical_section(); > } > > This checker will have false positives if someone uses this pattern > to legitimately invoke a destructor immediately (or the statement is > at the end of a scope anyway). To reduce the number we ignore this > pattern in macros (this is heavily used by gtest) and ignore objects > with no user-defined destructor. > > Reviewers: alexfh, djasper > > Subscribers: cfe-commits > > Differential Revision: http://reviews.llvm.org/D4615 > > Added: > clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.cpp > clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.h > clang-tools-extra/trunk/test/clang-tidy/misc-unused-raii.cpp > Modified: > clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt > clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp > > Modified: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt?rev=213647&r1=213646&r2=213647&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt (original) > +++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt Tue Jul 22 > 07:30:35 2014 > @@ -6,6 +6,7 @@ add_clang_library(clangTidyMiscModule > MiscTidyModule.cpp > RedundantSmartptrGet.cpp > SwappedArgumentsCheck.cpp > + UnusedRAII.cpp > UseOverride.cpp > > LINK_LIBS > > Modified: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp?rev=213647&r1=213646&r2=213647&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp (original) > +++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp Tue Jul 22 > 07:30:35 2014 > @@ -14,6 +14,7 @@ > #include "BoolPointerImplicitConversion.h" > #include "RedundantSmartptrGet.h" > #include "SwappedArgumentsCheck.h" > +#include "UnusedRAII.h" > #include "UseOverride.h" > > namespace clang { > @@ -35,6 +36,9 @@ public: > "misc-swapped-arguments", > new ClangTidyCheckFactory<SwappedArgumentsCheck>()); > CheckFactories.addCheckFactory( > + "misc-unused-raii", > + new ClangTidyCheckFactory<UnusedRAIICheck>()); > + CheckFactories.addCheckFactory( > "misc-use-override", > new ClangTidyCheckFactory<UseOverride>()); > } > > Added: clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.cpp?rev=213647&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.cpp (added) > +++ clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.cpp Tue Jul 22 > 07:30:35 2014 > @@ -0,0 +1,83 @@ > +//===--- UnusedRAII.cpp - clang-tidy ---------------------------===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > > +//===----------------------------------------------------------------------===// > + > +#include "UnusedRAII.h" > +#include "clang/AST/ASTContext.h" > +#include "clang/Lex/Lexer.h" > + > +using namespace clang::ast_matchers; > + > +namespace clang { > +namespace ast_matchers { > +AST_MATCHER(CXXRecordDecl, hasUserDeclaredDestructor) { > + // TODO: If the dtor is there but empty we don't want to warn either. > + return Node.hasUserDeclaredDestructor(); > +} > +} // namespace ast_matchers > + > +namespace tidy { > + > +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 = bindTemporaryExpr(unless(has(callExpr()))).bind("temp"); > + Finder->addMatcher( > + exprWithCleanups( > + unless(hasAncestor(decl( > + anyOf(recordDecl(ast_matchers::isTemplateInstantiation()), > + > functionDecl(ast_matchers::isTemplateInstantiation()))))), > + hasParent(compoundStmt().bind("compound")), > + hasDescendant(typeLoc().bind("typeloc")), > + hasType(recordDecl(hasUserDeclaredDestructor())), > + anyOf(has(BindTemp), has(functionalCastExpr(has(BindTemp))))) > + .bind("expr"), > + this); > +} > + > +void UnusedRAIICheck::check(const MatchFinder::MatchResult &Result) { > + const auto *E = Result.Nodes.getStmtAs<Expr>("expr"); > + > + // We ignore code expanded from macros to reduce the number of false > + // positives. > + if (E->getLocStart().isMacroID()) > + return; > + > + // Don't emit a warning for the last statement in the surrounding > compund > + // statement. > + const auto *CS = Result.Nodes.getStmtAs<CompoundStmt>("compound"); > + if (E == CS->body_back()) > + return; > + > + // Emit a warning. > + auto D = diag(E->getLocStart(), "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.getStmtAs<CXXBindTemporaryExpr>("temp"); > + if (const auto *TOE = > dyn_cast<CXXTemporaryObjectExpr>(BTE->getSubExpr())) > + if (TOE->getNumArgs() == 0) { > + D << FixItHint::CreateReplacement( > + CharSourceRange::getTokenRange(TOE->getParenOrBraceRange()), > + Replacement); > + return; > + } > + > + // Otherwise just suggest adding a name. > + const auto *TL = Result.Nodes.getNodeAs<TypeLoc>("typeloc"); > + D << FixItHint::CreateInsertion( > + Lexer::getLocForEndOfToken(TL->getLocEnd(), 0, > *Result.SourceManager, > + Result.Context->getLangOpts()), > + Replacement); > +} > + > +} // namespace tidy > +} // namespace clang > > Added: clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.h > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.h?rev=213647&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.h (added) > +++ clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.h Tue Jul 22 > 07:30:35 2014 > @@ -0,0 +1,47 @@ > +//===--- UnusedRAII.h - clang-tidy ------------------------------*- C++ > -*-===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > > +//===----------------------------------------------------------------------===// > + > +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_RAII_H > +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_RAII_H > + > +#include "../ClangTidy.h" > + > +namespace clang { > +namespace tidy { > + > +/// \brief Finds temporaries that look like RAII objects. > +/// > +/// The canonical example for this is a scoped lock. > +/// \code > +/// { > +/// scoped_lock(&global_mutex); > +/// critical_section(); > +/// } > +/// \endcode > +/// The destructor of the scoped_lock is called before the > critical_section is > +/// entered, leaving it unprotected. > +/// > +/// We apply a number of heuristics to reduce the false positive count of > this > +/// check: > +/// - Ignore code expanded from macros. Testing frameworks make heavy > use of > +/// this. > +/// - Ignore types with no user-declared constructor. Those are very > unlikely > +/// to be RAII objects. > +/// - Ignore objects at the end of a compound statement (doesn't change > behavior). > +/// - Ignore objects returned from a call. > +class UnusedRAIICheck : public ClangTidyCheck { > +public: > + void registerMatchers(ast_matchers::MatchFinder *Finder) override; > + void check(const ast_matchers::MatchFinder::MatchResult &Result) > override; > +}; > + > +} // namespace tidy > +} // namespace clang > + > +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_RAII_H > > Added: clang-tools-extra/trunk/test/clang-tidy/misc-unused-raii.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-unused-raii.cpp?rev=213647&view=auto > > ============================================================================== > --- clang-tools-extra/trunk/test/clang-tidy/misc-unused-raii.cpp (added) > +++ clang-tools-extra/trunk/test/clang-tidy/misc-unused-raii.cpp Tue Jul > 22 07:30:35 2014 > @@ -0,0 +1,61 @@ > +// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-unused-raii %t > +// REQUIRES: shell > + > +struct Foo { > + Foo(); > + Foo(int); > + Foo(int, int); > + ~Foo(); > +}; > + > +struct Bar { > + Bar(); > + Foo f; > +}; > + > +template <typename T> > +void qux() { > + T(42); > +} > + > +template <typename T> > +struct TFoo { > + TFoo(T); > + ~TFoo(); > +}; > + > +Foo f(); > + > +struct Ctor { > + Ctor(int); > + Ctor() { > + Ctor(0); // TODO: warn here. > + } > +}; > + > +void test() { > + Foo(42); > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately > after creation; did you mean to name the object? > +// CHECK-FIXES: Foo give_me_a_name(42); > + Foo(23, 42); > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately > after creation; did you mean to name the object? > +// CHECK-FIXES: Foo give_me_a_name(23, 42); > + Foo(); > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately > after creation; did you mean to name the object? > +// CHECK-FIXES: Foo give_me_a_name; > + TFoo<int>(23); > +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately > after creation; did you mean to name the object? > +// CHECK-FIXES: TFoo<int> give_me_a_name(23); > This test is failing on one of our bots: misc-unused-raii.cpp:39:17: error: expected string not found in input // CHECK-FIXES: Foo give_me_a_name(42); ^ misc-unused-raii.cpp.tmp.cpp:1:1: note: scanning from here // ^ misc-unused-raii.cpp.tmp.cpp:37:3: note: possible intended match here Foo(42); ^
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
