[clang-tools-extra] r213647 - [clang-tidy] Add a check for RAII temporaries.
Richard Smith
richard at metafoo.co.uk
Tue Jul 22 21:22:15 PDT 2014
Looks like the failure is due to clang-tidy asserting:
assertion failed at
third_party/llvm/llvm/tools/clang/include/clang/AST/ASTTypeTraits.h:211 in
bool clang::ast_type_traits::DynTypedNode::operator<(const
clang::ast_type_traits::DynTypedNode &) const: getMemoizationData() &&
Other.getMemoizationData()
Reverted in r213722.
Unhelpfully, test/clang-tidy/check_clang_tidy_fix.sh swallows this
information, and the test only fails because the input file was not
actually modified. The test runner script should also be fixed to fail if
clang-tidy crashes.
On Tue, Jul 22, 2014 at 5:40 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> 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/ed86b273/attachment.html>
More information about the cfe-commits
mailing list