[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 llvm-svn: 213647
This commit is contained in:
parent
8f41c3eb74
commit
8ca8651171
|
@ -6,6 +6,7 @@ add_clang_library(clangTidyMiscModule
|
||||||
MiscTidyModule.cpp
|
MiscTidyModule.cpp
|
||||||
RedundantSmartptrGet.cpp
|
RedundantSmartptrGet.cpp
|
||||||
SwappedArgumentsCheck.cpp
|
SwappedArgumentsCheck.cpp
|
||||||
|
UnusedRAII.cpp
|
||||||
UseOverride.cpp
|
UseOverride.cpp
|
||||||
|
|
||||||
LINK_LIBS
|
LINK_LIBS
|
||||||
|
|
|
@ -14,6 +14,7 @@
|
||||||
#include "BoolPointerImplicitConversion.h"
|
#include "BoolPointerImplicitConversion.h"
|
||||||
#include "RedundantSmartptrGet.h"
|
#include "RedundantSmartptrGet.h"
|
||||||
#include "SwappedArgumentsCheck.h"
|
#include "SwappedArgumentsCheck.h"
|
||||||
|
#include "UnusedRAII.h"
|
||||||
#include "UseOverride.h"
|
#include "UseOverride.h"
|
||||||
|
|
||||||
namespace clang {
|
namespace clang {
|
||||||
|
@ -34,6 +35,9 @@ public:
|
||||||
CheckFactories.addCheckFactory(
|
CheckFactories.addCheckFactory(
|
||||||
"misc-swapped-arguments",
|
"misc-swapped-arguments",
|
||||||
new ClangTidyCheckFactory<SwappedArgumentsCheck>());
|
new ClangTidyCheckFactory<SwappedArgumentsCheck>());
|
||||||
|
CheckFactories.addCheckFactory(
|
||||||
|
"misc-unused-raii",
|
||||||
|
new ClangTidyCheckFactory<UnusedRAIICheck>());
|
||||||
CheckFactories.addCheckFactory(
|
CheckFactories.addCheckFactory(
|
||||||
"misc-use-override",
|
"misc-use-override",
|
||||||
new ClangTidyCheckFactory<UseOverride>());
|
new ClangTidyCheckFactory<UseOverride>());
|
||||||
|
|
|
@ -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
|
|
@ -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
|
|
@ -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);
|
||||||
|
|
||||||
|
Bar();
|
||||||
|
f();
|
||||||
|
qux<Foo>();
|
||||||
|
|
||||||
|
#define M Foo();
|
||||||
|
M
|
||||||
|
|
||||||
|
{
|
||||||
|
Foo();
|
||||||
|
}
|
||||||
|
Foo();
|
||||||
|
}
|
Loading…
Reference in New Issue