[clang-tidy] Simplify unused RAII check

[steveire]

Summary

Fix handling of default construction where the constructor has a default arg.

[njames93]

clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-raii.cpp

49–53

Its not obvious from the code, but this should work if there are multiple default args. Can that behaviour be demonstrated?

[steveire]

Update

[njames93]

LG, with 1 nit.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-raii.cpp

81–82

Is this necessary?

clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.h

 public:
   UnusedRaiiCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNUSEDRAIICHECK_H

clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp

 AST_MATCHER(CXXRecordDecl, hasNonTrivialDestructor) {
   // TODO: If the dtor is there but empty we don't want to warn either.
   return Node.hasDefinition() && Node.hasNonTrivialDestructor();
 }
 } // namespace
 
 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 = cxxBindTemporaryExpr(
-                      unless(has(ignoringParenImpCasts(callExpr()))),
-                      unless(has(ignoringParenImpCasts(objcMessageExpr()))))
-                      .bind("temp");
+  // destroyed.
   Finder->addMatcher(
-      traverse(TK_AsIs,
-               exprWithCleanups(
-                   unless(isInTemplateInstantiation()),
-                   hasParent(compoundStmt().bind("compound")),
-                   hasType(cxxRecordDecl(hasNonTrivialDestructor())),
-                   anyOf(has(ignoringParenImpCasts(BindTemp)),
-                         has(ignoringParenImpCasts(cxxFunctionalCastExpr(
-                             has(ignoringParenImpCasts(BindTemp)))))))
-                   .bind("expr")),
+      cxxConstructExpr(hasParent(compoundStmt().bind("compound")),
+                       hasType(cxxRecordDecl(hasNonTrivialDestructor())))
+          .bind("expr"),
       this);
 }
 
 void UnusedRaiiCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *E = Result.Nodes.getNodeAs<Expr>("expr");
+  const auto *E = Result.Nodes.getNodeAs<CXXConstructExpr>("expr");
 
   // We ignore code expanded from macros to reduce the number of false
   // positives.
   if (E->getBeginLoc().isMacroID())
     return;
 
   // Don't emit a warning for the last statement in the surrounding compound
   // statement.
   const auto *CS = Result.Nodes.getNodeAs<CompoundStmt>("compound");
-  if (E == CS->body_back())
+  const auto *LastExpr = dyn_cast<Expr>(CS->body_back());
+  if (LastExpr && E == LastExpr->IgnoreUnlessSpelledInSource())
     return;
 
   // Emit a warning.
   auto D = diag(E->getBeginLoc(), "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.getNodeAs<CXXBindTemporaryExpr>("temp");
-  if (const auto *TOE = dyn_cast<CXXTemporaryObjectExpr>(BTE->getSubExpr()))
-    if (TOE->getNumArgs() == 0) {
-      D << FixItHint::CreateReplacement(
-          CharSourceRange::getTokenRange(TOE->getParenOrBraceRange()),
-          Replacement);
-      return;
-    }
+  if (E->getNumArgs() == 0 || isa<CXXDefaultArgExpr>(E->getArg(0))) {
+        CharSourceRange::getTokenRange(E->getParenOrBraceRange()), Replacement);
 
   // Otherwise just suggest adding a name. To find the place to insert the name
   // find the first TypeLoc in the children of E, which always points to the
   // written type.
-  auto Matches =
-      match(expr(hasDescendant(typeLoc().bind("t"))), *E, *Result.Context);
-  if (const auto *TL = selectFirst<TypeLoc>("t", Matches))
-    D << FixItHint::CreateInsertion(
-        Lexer::getLocForEndOfToken(TL->getEndLoc(), 0, *Result.SourceManager,
-                                   getLangOpts()),
+  D << FixItHint::CreateInsertion(E->getParenOrBraceRange().getBegin(),
                                   Replacement);
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/bugprone-unused-raii.cpp

void templ() {
   T();
 }
 
 template <typename T>
 void neverInstantiated() {
   T();
 }
 
+struct CtorDefaultArg {
+  CtorDefaultArg(int i = 0);
+  ~CtorDefaultArg();
+};
+
 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);
 
   FooBar();
 // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
 // CHECK-FIXES: FooBar give_me_a_name;
 
+  CtorDefaultArg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+  // CHECK-FIXES: CtorDefaultArg give_me_a_name;
+
   templ<FooBar>();
   templ<Bar>();
 
   Bar();
   f();
   qux<Foo>();
 
 #define M Foo();
   M
 
   {
     Foo();
   }
   Foo();
 }