[clang-tidy] Add a checker for code that looks like a delegate constructors but doesn't delegate.

[bkramer]

Summary

class Foo {

Foo() {
  Foo(42); // oops
}
Foo(int);

};

This is valid code but it does nothing and we can't emit a warning in clang
because there might be side effects. The checker emits a warning for this
pattern and also for base class initializers written in this style.

There is some overlap with the unused-rtti checker but they follow different
goals and fire in different places most of the time.

[alexfh]

clang-tidy/misc/CMakeLists.txt

9

I'd add "Check" to the class name for consistency.

clang-tidy/misc/UndelegatedConstructor.cpp

63

Please add a test with template instantiations. It's useful to know that the matcher doesn't crash on them ;)

72

The message could be a bit clearer of what actually happens here. Maybe something along the lines of: "Did you intend to call a delegated constructor? A temporary object is created here instead"

test/clang-tidy/misc-undelegated-constructor.cpp

1

Please add -implicit-check-not='{{warning:|error:}}'.

[bkramer]

Closed by commit rL214397 (authored by d0k).

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   BoolPointerImplicitConversion.cpp
   MiscTidyModule.cpp
   RedundantSmartptrGet.cpp
   SwappedArgumentsCheck.cpp
+  UndelegatedConstructor.cpp
   UnusedRAII.cpp
   UseOverride.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "BoolPointerImplicitConversion.h"
 #include "RedundantSmartptrGet.h"
 #include "SwappedArgumentsCheck.h"
+#include "UndelegatedConstructor.h"
 #include "UnusedRAII.h"
 #include "UseOverride.h"
 
 namespace clang {
 namespace tidy {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
         "misc-argument-comment",
         new ClangTidyCheckFactory<ArgumentCommentCheck>());
     CheckFactories.addCheckFactory(
         "misc-bool-pointer-implicit-conversion",
         new ClangTidyCheckFactory<BoolPointerImplicitConversion>());
     CheckFactories.addCheckFactory(
         "misc-redundant-smartptr-get",
         new ClangTidyCheckFactory<RedundantSmartptrGet>());
     CheckFactories.addCheckFactory(
         "misc-swapped-arguments",
         new ClangTidyCheckFactory<SwappedArgumentsCheck>());
     CheckFactories.addCheckFactory(
+        "misc-undelegated-constructor",
+        new ClangTidyCheckFactory<UndelegatedConstructor>());
+    CheckFactories.addCheckFactory(
         "misc-unused-raii",
         new ClangTidyCheckFactory<UnusedRAIICheck>());
     CheckFactories.addCheckFactory(
         "misc-use-override",
         new ClangTidyCheckFactory<UseOverride>());
   }
 };
 

clang-tidy/misc/UndelegatedConstructor.h

+//===--- UndelegatedConstructor.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_UNDELEGATED_CONSTRUCTOR_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNDELEGATED_CONSTRUCTOR_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Finds creation of temporary objects in constructors that look like a
+/// function call to another constructor of the same class. The user most likely
+/// meant to use a delegating constructor or base class initializer.
+class UndelegatedConstructor : 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_UNDELEGATED_CONSTRUCTOR_H

clang-tidy/misc/UndelegatedConstructor.cpp

+//===--- UndelegatedConstructor.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 "UndelegatedConstructor.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+
+namespace ast_matchers {
+AST_MATCHER_P(Stmt, ignoringTemporaryExpr, internal::Matcher<Stmt>,
+              InnerMatcher) {
+  const Stmt *E = &Node;
+  for (;;) {
+    // Temporaries with non-trivial dtors.
+    if (const auto *EWC = dyn_cast<ExprWithCleanups>(E))
+      E = EWC->getSubExpr();
+    // Temporaries with zero or more than two ctor arguments.
+    else if (const auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
+      E = BTE->getSubExpr();
+    // Temporaries with exactly one ctor argument.
+    else if (const auto *FCE = dyn_cast<CXXFunctionalCastExpr>(E))
+      E = FCE->getSubExpr();
+    else
+      break;
+  }
+
+  return InnerMatcher.matches(*E, Finder, Builder);
+}
+
+// Finds a node if it's a base of an already bound node.
+AST_MATCHER_P(CXXRecordDecl, baseOfBoundNode, std::string, ID) {
+  return Builder->removeBindings([&](const internal::BoundNodesMap &Nodes) {
+    const auto *Derived = Nodes.getNodeAs<CXXRecordDecl>(ID);
+    return Derived != &Node && !Derived->isDerivedFrom(&Node);
+  });
+}
+} // namespace ast_matchers
+
+namespace tidy {
+
+void UndelegatedConstructor::registerMatchers(MatchFinder *Finder) {
+  // We look for calls to constructors of the same type in constructors. To do
+  // this we have to look through a variety of nodes that occur in the path,
+  // depending on the type's destructor and the number of arguments on the
+  // constructor call, this is handled by ignoringTemporaryExpr. Ignore template
+  // instantiations to reduce the number of duplicated warnings.
+  Finder->addMatcher(
+      compoundStmt(
+          hasParent(constructorDecl(ofClass(recordDecl().bind("parent")))),
+          forEach(ignoringTemporaryExpr(
+              constructExpr(hasDeclaration(constructorDecl(ofClass(
+                                recordDecl(baseOfBoundNode("parent"))))))
+                  .bind("construct"))),
+          unless(hasAncestor(decl(
+              anyOf(recordDecl(ast_matchers::isTemplateInstantiation()),
+                    functionDecl(ast_matchers::isTemplateInstantiation())))))),
+      this);
+}
+
+void UndelegatedConstructor::check(const MatchFinder::MatchResult &Result) {
+  const auto *E = Result.Nodes.getStmtAs<CXXConstructExpr>("construct");
+  diag(E->getLocStart(),
+       "calling a different constructor in a constructor does not chain them");
+}
+
+} // namespace tidy
+} // namespace clang

test/clang-tidy/misc-undelegated-constructor.cpp

+// RUN: clang-tidy -checks='-*,misc-undelegated-constructor' %s -- -std=c++11 2>&1 | FileCheck %s
+
+struct Ctor;
+Ctor foo();
+
+struct Ctor {
+  Ctor();
+  Ctor(int);
+  Ctor(int, int);
+  Ctor(Ctor *i) {
+    Ctor();
+// CHECK: :[[@LINE-1]]:5: warning: calling a different constructor in a constructor does not chain them
+    Ctor(0);
+// CHECK: :[[@LINE-1]]:5: warning: calling a different constructor in a constructor does not chain them
+    Ctor(1, 2);
+// CHECK: :[[@LINE-1]]:5: warning: calling a different constructor in a constructor does not chain them
+    foo();
+  }
+};
+
+Ctor::Ctor() {
+  Ctor(1);
+// CHECK: :[[@LINE-1]]:3: warning: calling a different constructor in a constructor does not chain them
+}
+
+Ctor::Ctor(int i) : Ctor(i, 1) {} // properly delegated.
+
+struct Dtor {
+  Dtor();
+  Dtor(int);
+  Dtor(int, int);
+  Dtor(Ctor *i) {
+    Dtor();
+// CHECK: :[[@LINE-1]]:5: warning: calling a different constructor in a constructor does not chain them
+    Dtor(0);
+// CHECK: :[[@LINE-1]]:5: warning: calling a different constructor in a constructor does not chain them
+    Dtor(1, 2);
+// CHECK: :[[@LINE-1]]:5: warning: calling a different constructor in a constructor does not chain them
+  }
+  ~Dtor();
+};
+
+struct Base {};
+struct Derived : public Base {
+  Derived() { Base(); }
+// CHECK: :[[@LINE-1]]:15: warning: calling a different constructor in a constructor does not chain them
+};