[clang-tools-extra] r213647 - [clang-tidy] Add a check for RAII temporaries.
Richard Smith
richard at metafoo.co.uk
Tue Jul 22 17:40:37 PDT 2014
On Tue, Jul 22, 2014 at 5:30 AM, Benjamin Kramer <benny.kra at googlemail.com>
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);
^
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140722/ff800155/attachment.html>
More information about the cfe-commits
mailing list