On Wed, Jul 23, 2014 at 4:49 AM, Benjamin Kramer
<[email protected]> wrote:
> Author: d0k
> Date: Wed Jul 23 06:49:46 2014
> New Revision: 213738
>
> URL: http://llvm.org/viewvc/llvm-project?rev=213738&view=rev
> Log:
> Reapply r213647 with a fix.
>
> ASTMatchers currently have problems mixing bound TypeLoc nodes with Decl/Stmt
> nodes. That should be fixed soon but for this checker there we only need the
> TypeLoc to generate a fixit so postpone the potentially heavyweight AST
> walking
> until after we know that we're going to emit a warning.
>
> This is covered by existing test cases.
Though this failure didn't turn up for you locally before the original
commit? (does it turn up on all buildbots, and not on local builds for
some reason?) Is there any test we could add that would help
demonstrate this failure sooner/more pervasively (usually if we see,
say, a architecture-specific failure we add an test with the specified
architecture hardcoded so that it reproduces locally for everyone
rather than relying on the coverage of a particular buildbot)
>
> Original message:
> [clang-tidy] Add a check for RAII temporaries.
>
> 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.
>
> 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=213738&r1=213737&r2=213738&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt (original)
> +++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt Wed Jul 23
> 06:49:46 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=213738&r1=213737&r2=213738&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp (original)
> +++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp Wed Jul 23
> 06:49:46 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=213738&view=auto
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.cpp (added)
> +++ clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.cpp Wed Jul 23
> 06:49:46 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=213738&view=auto
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.h (added)
> +++ clang-tools-extra/trunk/clang-tidy/misc/UnusedRAII.h Wed Jul 23 06:49:46
> 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=213738&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 Wed Jul 23
> 06:49:46 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);
> +
> + Bar();
> + f();
> + qux<Foo>();
> +
> +#define M Foo();
> + M
> +
> + {
> + Foo();
> + }
> + Foo();
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
