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