[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")),
+      mapAnyOf(cxxConstructExpr, cxxUnresolvedConstructExpr)
+          .with(hasParent(compoundStmt().bind("compound")),
+                anyOf(hasType(cxxRecordDecl(hasNonTrivialDestructor())),
+                      hasType(templateSpecializationType(
+                          hasDeclaration(classTemplateDecl(has(
+                              cxxRecordDecl(hasNonTrivialDestructor()))))))))
+          .bind("expr"),
       this);
 }
 
+template <typename T>
+void reportDiagnostic(DiagnosticBuilder D, const T *Node, SourceRange SR,
+                      bool DefaultConstruction) {
+  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.
+  if (DefaultConstruction) {
+    D << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(SR),
+                                      Replacement);
+    return;
+  }
+
+  // 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.
+  D << FixItHint::CreateInsertion(SR.getBegin(), Replacement);
+}
+
 void UnusedRaiiCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *E = Result.Nodes.getNodeAs<Expr>("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;
-    }
-
-  // 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()),
-        Replacement);
+  if (const auto *Node = dyn_cast<CXXConstructExpr>(E))
+    reportDiagnostic(D, Node, Node->getParenOrBraceRange(),
+                     Node->getNumArgs() == 0 ||
+                         isa<CXXDefaultArgExpr>(Node->getArg(0)));
+  if (const auto *Node = dyn_cast<CXXUnresolvedConstructExpr>(E)) {
+    auto SR = SourceRange(Node->getLParenLoc(), Node->getRParenLoc());
+    auto DefaultConstruction = Node->getNumArgs() == 0;
+    if (!DefaultConstruction) {
+      auto FirstArg = Node->getArg(0);
+      DefaultConstruction = isa<CXXDefaultArgExpr>(FirstArg);
+      if (auto ILE = dyn_cast<InitListExpr>(FirstArg)) {
+        DefaultConstruction = ILE->getNumInits() == 0;
+        SR = SourceRange(ILE->getLBraceLoc(), ILE->getRBraceLoc());
+      }
+    }
+    reportDiagnostic(D, Node, SR, DefaultConstruction);
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

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

-// RUN: %check_clang_tidy %s bugprone-unused-raii %t
+// RUN: %check_clang_tidy %s bugprone-unused-raii %t -- -- -fno-delayed-template-parsing
 
 struct Foo {
   Foo();
   Foo(int);
   Foo(int, int);
   ~Foo();
 };
 
void templ() {
   T();
 }
 
 template <typename T>
 void neverInstantiated() {
   T();
 }
 
+struct CtorDefaultArg {
+  CtorDefaultArg(int i = 0);
+  ~CtorDefaultArg();
+};
+
+template <typename T>
+struct TCtorDefaultArg {
+  TCtorDefaultArg(T i = 0);
+  ~TCtorDefaultArg();
+};
+
+struct CtorTwoDefaultArg {
+  CtorTwoDefaultArg(int i = 0, bool b = false);
+  ~CtorTwoDefaultArg();
+};
+
+template <typename T>
+void templatetest() {
+  TCtorDefaultArg<T>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+  // CHECK-FIXES: TCtorDefaultArg<T> give_me_a_name;
+  TCtorDefaultArg<T>{};
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+  // CHECK-FIXES: TCtorDefaultArg<T> give_me_a_name;
+
+  TCtorDefaultArg<T>(T{});
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+  // CHECK-FIXES: TCtorDefaultArg<T> give_me_a_name(T{});
+  TCtorDefaultArg<T>{T{}};
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+  // CHECK-FIXES: TCtorDefaultArg<T> give_me_a_name{T{}};
+
+  int i = 0;
+  (void)i;
+}
+
 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-FIXES: FooBar give_me_a_name;
 
   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};
   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;
+
+  CtorTwoDefaultArg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+  // CHECK-FIXES: CtorTwoDefaultArg give_me_a_name;
+
+  TCtorDefaultArg<int>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+  // CHECK-FIXES: TCtorDefaultArg<int> give_me_a_name;
+
+  TCtorDefaultArg<int>{};
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: object destroyed immediately after creation; did you mean to name the object?
+  // CHECK-FIXES: TCtorDefaultArg<int> give_me_a_name;
+
   templ<FooBar>();
   templ<Bar>();
 
   Bar();
   f();
   qux<Foo>();
 
 #define M Foo();
   M
 
   {
     Foo();
   }
   Foo();
 }