From 11887924806547ec02e0838644afa80c69084fa8 Mon Sep 17 00:00:00 2001 From: Benjamin Kramer Date: Thu, 31 Jul 2014 09:58:52 +0000 Subject: [PATCH] [clang-tidy] Add a checker for code that looks like a delegate constructors but doesn't delegate. 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. Reviewers: alexfh, djasper Subscribers: cfe-commits Differential Revision: http://reviews.llvm.org/D4667 llvm-svn: 214397 --- clang-tools-extra/clang-tidy/misc/CMakeLists.txt | 1 + .../clang-tidy/misc/MiscTidyModule.cpp | 4 ++ .../clang-tidy/misc/UndelegatedConstructor.cpp | 76 ++++++++++++++++++++++ .../clang-tidy/misc/UndelegatedConstructor.h | 30 +++++++++ .../clang-tidy/misc-undelegated-constructor.cpp | 54 +++++++++++++++ 5 files changed, 165 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/misc/UndelegatedConstructor.cpp create mode 100644 clang-tools-extra/clang-tidy/misc/UndelegatedConstructor.h create mode 100644 clang-tools-extra/test/clang-tidy/misc-undelegated-constructor.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 5614d12..fbe5bb0 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -6,6 +6,7 @@ add_clang_library(clangTidyMiscModule MiscTidyModule.cpp RedundantSmartptrGet.cpp SwappedArgumentsCheck.cpp + UndelegatedConstructor.cpp UnusedRAII.cpp UseOverride.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index e3dbdc3..8cf70d3 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -14,6 +14,7 @@ #include "BoolPointerImplicitConversion.h" #include "RedundantSmartptrGet.h" #include "SwappedArgumentsCheck.h" +#include "UndelegatedConstructor.h" #include "UnusedRAII.h" #include "UseOverride.h" @@ -36,6 +37,9 @@ public: "misc-swapped-arguments", new ClangTidyCheckFactory()); CheckFactories.addCheckFactory( + "misc-undelegated-constructor", + new ClangTidyCheckFactory()); + CheckFactories.addCheckFactory( "misc-unused-raii", new ClangTidyCheckFactory()); CheckFactories.addCheckFactory( diff --git a/clang-tools-extra/clang-tidy/misc/UndelegatedConstructor.cpp b/clang-tools-extra/clang-tidy/misc/UndelegatedConstructor.cpp new file mode 100644 index 0000000..7e31e1b --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/UndelegatedConstructor.cpp @@ -0,0 +1,76 @@ +//===--- 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, + InnerMatcher) { + const Stmt *E = &Node; + for (;;) { + // Temporaries with non-trivial dtors. + if (const auto *EWC = dyn_cast(E)) + E = EWC->getSubExpr(); + // Temporaries with zero or more than two ctor arguments. + else if (const auto *BTE = dyn_cast(E)) + E = BTE->getSubExpr(); + // Temporaries with exactly one ctor argument. + else if (const auto *FCE = dyn_cast(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(ID); + return Derived != &Node && !Derived->isDerivedFrom(&Node); + }); +} +} // namespace ast_matchers + +namespace tidy { + +void UndelegatedConstructorCheck::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 UndelegatedConstructorCheck::check(const MatchFinder::MatchResult &Result) { + const auto *E = Result.Nodes.getStmtAs("construct"); + diag(E->getLocStart(), "did you intend to call a delegated constructor? " + "A temporary object is created here instead"); +} + +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/misc/UndelegatedConstructor.h b/clang-tools-extra/clang-tidy/misc/UndelegatedConstructor.h new file mode 100644 index 0000000..1e24d08 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/UndelegatedConstructor.h @@ -0,0 +1,30 @@ +//===--- 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 UndelegatedConstructorCheck : 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 diff --git a/clang-tools-extra/test/clang-tidy/misc-undelegated-constructor.cpp b/clang-tools-extra/test/clang-tidy/misc-undelegated-constructor.cpp new file mode 100644 index 0000000..09d683b --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-undelegated-constructor.cpp @@ -0,0 +1,54 @@ +// RUN: clang-tidy -checks='-*,misc-undelegated-constructor' %s -- -std=c++11 2>&1 | FileCheck %s -implicit-check-not='{{warning:|error:}}' + +struct Ctor; +Ctor foo(); + +struct Ctor { + Ctor(); + Ctor(int); + Ctor(int, int); + Ctor(Ctor *i) { + Ctor(); +// CHECK: :[[@LINE-1]]:5: warning: did you intend to call a delegated constructor? A temporary object is created here instead + Ctor(0); +// CHECK: :[[@LINE-1]]:5: warning: did you intend to call a delegated constructor? A temporary object is created here instead + Ctor(1, 2); +// CHECK: :[[@LINE-1]]:5: warning: did you intend to call a delegated constructor? A temporary object is created here instead + foo(); + } +}; + +Ctor::Ctor() { + Ctor(1); +// CHECK: :[[@LINE-1]]:3: warning: did you intend to call a delegated constructor? A temporary object is created here instead +} + +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: did you intend to call a delegated constructor? A temporary object is created here instead + Dtor(0); +// CHECK: :[[@LINE-1]]:5: warning: did you intend to call a delegated constructor? A temporary object is created here instead + Dtor(1, 2); +// CHECK: :[[@LINE-1]]:5: warning: did you intend to call a delegated constructor? A temporary object is created here instead + } + ~Dtor(); +}; + +struct Base {}; +struct Derived : public Base { + Derived() { Base(); } +// CHECK: :[[@LINE-1]]:15: warning: did you intend to call a delegated constructor? A temporary object is created here instead +}; + +template +struct TDerived : public Base { + TDerived() { Base(); } +}; + +TDerived t; -- 2.7.4