[clang-tools-extra] r213738 - Reapply r213647 with a fix.

[Benjamin Kramer]

On Wed, Jul 23, 2014 at 5:26 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Wed, Jul 23, 2014 at 4:49 AM, Benjamin Kramer
> <benny.kra at googlemail.com> 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)

I still don't fully understand under what circumstances this failed, I
was able to reproduce it locally but not with a standard debug build.
Daniel has fixed the root cause (r213751) though and that comes with a
better test case for ASTMatchers that should fail reliably.

-Ben

>>
>> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits