Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -3,6 +3,7 @@ add_clang_library(clangTidyMiscModule ArgumentCommentCheck.cpp AssertSideEffectCheck.cpp + ForwardingReferenceOverloadCheck.cpp MisplacedConstCheck.cpp UnconventionalAssignOperatorCheck.cpp BoolPointerImplicitConversionCheck.cpp Index: clang-tidy/misc/ForwardingReferenceOverloadCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/ForwardingReferenceOverloadCheck.h @@ -0,0 +1,42 @@ +//===--- ForwardingReferenceOverloadCheck.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_FORWARDING_REFERENCE_OVERLOAD_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDING_REFERENCE_OVERLOAD_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// The checker looks for constructors that can act as copy or move constructors +/// through their forwarding reference parameters. If a non const lvalue +/// reference is passed to the constructor, the forwarding reference parameter +/// can be a perfect match while the const reference parameter of the copy +/// constructor can't. The forwarding reference constructor will be called, +/// which can lead to confusion. +/// For detailed description of this problem see: Scott Meyers, Effective Modern +/// C++ Design, item 26. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-forwarding-reference-overload.html +class ForwardingReferenceOverloadCheck : public ClangTidyCheck { +public: + ForwardingReferenceOverloadCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDING_REFERENCE_OVERLOAD_H Index: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp @@ -0,0 +1,145 @@ +//===--- ForwardingReferenceOverloadCheck.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 "ForwardingReferenceOverloadCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +namespace { +// Check if the given type is related to std::enable_if. +AST_MATCHER(QualType, isEnableIf) { + auto CheckTemplate = [](const TemplateSpecializationType *Spec) { + if (!Spec || !Spec->getTemplateName().getAsTemplateDecl()) { + return false; + } + const NamedDecl *TypeDecl = + Spec->getTemplateName().getAsTemplateDecl()->getTemplatedDecl(); + return TypeDecl->isInStdNamespace() && + (TypeDecl->getName().equals("enable_if") || + TypeDecl->getName().equals("enable_if_t")); + }; + const Type *BaseType = Node.getTypePtr(); + // Case: pointer or reference to enable_if. + while (BaseType->isPointerType() || BaseType->isReferenceType()) { + BaseType = BaseType->getPointeeType().getTypePtr(); + } + // Case: type parameter dependent (enable_if>). + if (const auto *Dependent = BaseType->getAs()) { + BaseType = Dependent->getQualifier()->getAsType(); + } + if (CheckTemplate(BaseType->getAs())) { + return true; // Case: enable_if_t< >. + } else if (const auto *Elaborated = BaseType->getAs()) { + if (const auto *Qualifier = Elaborated->getQualifier()->getAsType()) { + if (CheckTemplate(Qualifier->getAs())) { + return true; // Case: enable_if< >::type. + } + } + } + return false; +} +AST_MATCHER_P(TemplateTypeParmDecl, hasDefaultArgument, + clang::ast_matchers::internal::Matcher, TypeMatcher) { + return Node.hasDefaultArgument() && + TypeMatcher.matches(Node.getDefaultArgument(), Finder, Builder); +} +} + +void ForwardingReferenceOverloadCheck::registerMatchers(MatchFinder *Finder) { + // Forwarding references require C++11 or later. + if (!getLangOpts().CPlusPlus11) + return; + + auto ForwardingRefParm = + parmVarDecl( + hasType(qualType(rValueReferenceType(), + references(templateTypeParmType(hasDeclaration( + templateTypeParmDecl().bind("type-parm-decl")))), + unless(references(isConstQualified()))))) + .bind("parm-var"); + + DeclarationMatcher findOverload = + cxxConstructorDecl( + hasParameter(0, ForwardingRefParm), + unless(hasAnyParameter( + // No warning: enable_if as constructor parameter. + parmVarDecl(hasType(isEnableIf())))), + unless(hasParent(functionTemplateDecl(has(templateTypeParmDecl( + // No warning: enable_if as type parameter. + hasDefaultArgument(isEnableIf()))))))) + .bind("ctor"); + Finder->addMatcher(findOverload, this); +} + +void ForwardingReferenceOverloadCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *ParmVar = Result.Nodes.getNodeAs("parm-var"); + const auto *TypeParmDecl = + Result.Nodes.getNodeAs("type-parm-decl"); + + // Get the FunctionDecl and FunctionTemplateDecl containing the function + // parameter. + const auto *FuncForParam = dyn_cast(ParmVar->getDeclContext()); + if (!FuncForParam) + return; + const FunctionTemplateDecl *FuncTemplate = + FuncForParam->getDescribedFunctionTemplate(); + if (!FuncTemplate) + return; + + // Check that the template type parameter belongs to the same function + // template as the function parameter of that type. (This implies that type + // deduction will happen on the type.) + const TemplateParameterList *Params = FuncTemplate->getTemplateParameters(); + if (std::find(Params->begin(), Params->end(), TypeParmDecl) == Params->end()) + return; + + // Every parameter after the first must have a default value. + const auto *Ctor = Result.Nodes.getNodeAs("ctor"); + for (auto Iter = Ctor->param_begin() + 1; Iter != Ctor->param_end(); ++Iter) { + if (!(*Iter)->hasDefaultArg()) + return; + } + bool EnabledCopy = false, DisabledCopy = false, EnabledMove = false, + DisabledMove = false; + for (const auto *OtherCtor : Ctor->getParent()->ctors()) { + if (OtherCtor->isCopyOrMoveConstructor()) { + if (OtherCtor->isDeleted() || OtherCtor->getAccess() == AS_private) + (OtherCtor->isCopyConstructor() ? DisabledCopy : DisabledMove) = true; + else + (OtherCtor->isCopyConstructor() ? EnabledCopy : EnabledMove) = true; + } + } + bool Copy = !DisabledCopy || EnabledCopy, Move = !DisabledMove || EnabledMove; + if (!Copy && !Move) + return; + diag(Ctor->getLocation(), + "constructor accepting a forwarding reference can " + "hide the %select{copy|move|copy and move}0 constructor%s1") + << (Copy && Move ? 2 : (Copy ? 0 : 1)) << Copy + Move; + for (const auto *OtherCtor : Ctor->getParent()->ctors()) { + if (OtherCtor->isCopyOrMoveConstructor() && !OtherCtor->isDeleted() && + OtherCtor->getAccess() != AS_private) { + diag(OtherCtor->getLocation(), + "%select{copy|move}0 constructor declared here", DiagnosticIDs::Note) + << OtherCtor->isMoveConstructor(); + } + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -17,6 +17,7 @@ #include "DefinitionsInHeadersCheck.h" #include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" +#include "ForwardingReferenceOverloadCheck.h" #include "InaccurateEraseCheck.h" #include "IncorrectRoundings.h" #include "InefficientAlgorithmCheck.h" @@ -65,6 +66,8 @@ CheckFactories.registerCheck("misc-argument-comment"); CheckFactories.registerCheck( "misc-assert-side-effect"); + CheckFactories.registerCheck( + "misc-forwarding-reference-overload"); CheckFactories.registerCheck("misc-misplaced-const"); CheckFactories.registerCheck( "misc-unconventional-assign-operator"); Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -86,6 +86,11 @@ Finds and replaces explicit calls to the constructor in a return statement by a braced initializer list so that the return type is not needlessly repeated. + +- New `misc-forwarding-reference-overload + `_ check + + Finds perfect forwarding constructors that can unintentionally hide copy or move constructors. - Support clang-formatting of the code around applied fixes (``-format-style`` command-line option). Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -65,6 +65,7 @@ misc-definitions-in-headers misc-fold-init-type misc-forward-declaration-namespace + misc-forwarding-reference-overload misc-inaccurate-erase misc-incorrect-roundings misc-inefficient-algorithm Index: docs/clang-tidy/checks/misc-forwarding-reference-overload.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-forwarding-reference-overload.rst @@ -0,0 +1,52 @@ +.. title:: clang-tidy - misc-forwarding-reference-overload + +misc-forwarding-reference-overload +================================== + +The check looks for perfect forwarding constructors that can hide copy or move +constructors. If a non const lvalue reference is passed to the constructor, the +forwarding reference parameter will be a better match than the const reference +parameter of the copy constructor, so the perfect forwarding constructor will be +called, which can be confusing. +For detailed description of this issue see: Scott Meyers, Effective Modern C++, +Item 26. + +Consider the following example: + + .. code-block:: c++ + + class Person { + public: + // C1: perfect forwarding ctor + template + explicit Person(T&& n) {} + + // C2: perfect forwarding ctor with parameter default value + template + explicit Person(T&& n, int x = 1) {} + + // C3: perfect forwarding ctor guarded with enable_if + template,void>> + explicit Person(T&& n) {} + + // (possibly compiler generated) copy ctor + Person(const Person& rhs); + }; + +The check warns for constructors C1 and C2, because those can hide copy and move +constructors. We suppress warnings if the copy and the move constructors are both +disabled (deleted or private), because there is nothing the perfect forwarding +constructor could hide in this case. We also suppress warnings for constructors +like C3 that are guarded with an enable_if, assuming the programmer was aware of +the possible hiding. + +Background +---------- + +For deciding whether a constructor is guarded with enable_if, we consider the +default values of the type parameters and the types of the contructor +parameters. If any part of these types is std::enable_if or std::enable_if_t, we +assume the constructor is guarded. + + + Index: test/clang-tidy/misc-forwarding-reference-overload.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-forwarding-reference-overload.cpp @@ -0,0 +1,139 @@ +// RUN: %check_clang_tidy %s misc-forwarding-reference-overload %t + +namespace std { +template +struct enable_if {}; + +template +struct enable_if { typedef T type; }; + +template +using enable_if_t = typename enable_if::type; + +template +struct enable_if_nice { typedef T type; }; +} + +namespace foo { +template +struct enable_if { typedef T type; }; +} + +template +constexpr bool just_true = true; + +class Test1 { +public: + template + Test1(T &&n); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors [misc-forwarding-reference-overload] + + template + Test1(T &&n, int i = 5, ...); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors [misc-forwarding-reference-overload] + + template ::type> + Test1(T &&n); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors [misc-forwarding-reference-overload] + + template + Test1(T &&n, typename foo::enable_if::type i = 5, ...); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors [misc-forwarding-reference-overload] + + Test1(const Test1 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: note: copy constructor declared here + + Test1(Test1 &other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: note: copy constructor declared here + + Test1(Test1 &&other) {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: note: move constructor declared here +}; + +template +class Test2 { +public: + // Two parameters without default value, can't act as copy / move constructor. + template + Test2(T &&n, V &&m, int i = 5, ...); + + // Guarded with enable_if. + template + Test2(T &&n, int i = 5, std::enable_if_t a = 5, ...); + + // Guarded with enable_if. + template ::type &> + Test2(T &&n); + + // Guarded with enable_if. + template + Test2(T &&n, typename std::enable_if>::type **a = nullptr); + + // Guarded with enable_if. + template > *&&> + Test2(T &&n, double d = 0.0); + + // Not a forwarding reference parameter. + template + Test2(const T &&n); + + // Not a forwarding reference parameter. + Test2(int &&x); + + // Two parameters without default value, can't act as copy / move constructor. + template + Test2(T &&n, int x); + + // Not a forwarding reference parameter. + template + Test2(U &&n); +}; + +// The copy and move constructors are both disabled. +class Test3 { +public: + template + Test3(T &&n); + + template + Test3(T &&n, int I = 5, ...); + + Test3(const Test3 &rhs) = delete; + +private: + Test3(Test3 &&rhs); +}; + +// Both the copy and the (compiler generated) move constructors can be hidden. +class Test4 { +public: + template + Test4(T &&n); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors [misc-forwarding-reference-overload] + + Test4(const Test4 &rhs); + // CHECK-MESSAGES: :[[@LINE-1]]:3: note: copy constructor declared here +}; + +// Only the (compiler generated) copy constructor can be hidden. +class Test5 { +public: + template + Test5(T &&n); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy constructor [misc-forwarding-reference-overload] + + Test5(Test5 &&rhs) = delete; +}; + +// Only the move constructor can be hidden. +class Test6 { +public: + template + Test6(T &&n); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the move constructor [misc-forwarding-reference-overload] + + Test6(Test6 &&rhs); + // CHECK-MESSAGES: :[[@LINE-1]]:3: note: move constructor declared here +private: + Test6(const Test6 &rhs); +}; \ No newline at end of file