diff options
8 files changed, 385 insertions, 0 deletions
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index dd01f52b77e..36321e98c2e 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -3,6 +3,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyMiscModule ArgumentCommentCheck.cpp AssertSideEffectCheck.cpp + ForwardingReferenceOverloadCheck.cpp MisplacedConstCheck.cpp UnconventionalAssignOperatorCheck.cpp BoolPointerImplicitConversionCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp b/clang-tools-extra/clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp new file mode 100644 index 00000000000..550c116845b --- /dev/null +++ b/clang-tools-extra/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 <algorithm>
+
+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<is_integral<T>>).
+ if (const auto *Dependent = BaseType->getAs<DependentNameType>()) {
+ BaseType = Dependent->getQualifier()->getAsType();
+ }
+ if (CheckTemplate(BaseType->getAs<TemplateSpecializationType>())) {
+ return true; // Case: enable_if_t< >.
+ } else if (const auto *Elaborated = BaseType->getAs<ElaboratedType>()) {
+ if (const auto *Qualifier = Elaborated->getQualifier()->getAsType()) {
+ if (CheckTemplate(Qualifier->getAs<TemplateSpecializationType>())) {
+ return true; // Case: enable_if< >::type.
+ }
+ }
+ }
+ return false;
+}
+AST_MATCHER_P(TemplateTypeParmDecl, hasDefaultArgument,
+ clang::ast_matchers::internal::Matcher<QualType>, 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<ParmVarDecl>("parm-var");
+ const auto *TypeParmDecl =
+ Result.Nodes.getNodeAs<TemplateTypeParmDecl>("type-parm-decl");
+
+ // Get the FunctionDecl and FunctionTemplateDecl containing the function
+ // parameter.
+ const auto *FuncForParam = dyn_cast<FunctionDecl>(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<CXXConstructorDecl>("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
diff --git a/clang-tools-extra/clang-tidy/misc/ForwardingReferenceOverloadCheck.h b/clang-tools-extra/clang-tidy/misc/ForwardingReferenceOverloadCheck.h new file mode 100644 index 00000000000..ffdf0e84fa8 --- /dev/null +++ b/clang-tools-extra/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 diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index 755e2c7ad4a..43d09c75388 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/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 @@ public: CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment"); CheckFactories.registerCheck<AssertSideEffectCheck>( "misc-assert-side-effect"); + CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>( + "misc-forwarding-reference-overload"); CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const"); CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>( "misc-unconventional-assign-operator"); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 8f995c6fc12..87a0c9a0199 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -86,6 +86,11 @@ Improvements to clang-tidy 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 + <http://clang.llvm.org/extra/clang-tidy/checks/misc-forwarding-reference-overload.html>`_ 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). diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 8a35ac21035..a4a1b1891f6 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -77,6 +77,7 @@ Clang-Tidy Checks 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 diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst new file mode 100644 index 00000000000..434c6819ca9 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst @@ -0,0 +1,49 @@ +.. 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<typename T> + explicit Person(T&& n) {} + + // C2: perfect forwarding ctor with parameter default value + template<typename T> + explicit Person(T&& n, int x = 1) {} + + // C3: perfect forwarding ctor guarded with enable_if + template<typename T, typename X = enable_if_t<is_special<T>,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. diff --git a/clang-tools-extra/test/clang-tidy/misc-forwarding-reference-overload.cpp b/clang-tools-extra/test/clang-tidy/misc-forwarding-reference-overload.cpp new file mode 100644 index 00000000000..30e93472e57 --- /dev/null +++ b/clang-tools-extra/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 <bool B, class T = void> +struct enable_if {}; + +template <class T> +struct enable_if<true, T> { typedef T type; }; + +template <bool B, class T = void> +using enable_if_t = typename enable_if<B, T>::type; + +template <class T> +struct enable_if_nice { typedef T type; }; +} + +namespace foo { +template <class T> +struct enable_if { typedef T type; }; +} + +template <typename T> +constexpr bool just_true = true; + +class Test1 { +public: + template <typename T> + 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 <typename T> + 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 <typename T, typename U = typename std::enable_if_nice<T>::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 <typename T> + Test1(T &&n, typename foo::enable_if<long>::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 <typename U> +class Test2 { +public: + // Two parameters without default value, can't act as copy / move constructor. + template <typename T, class V> + Test2(T &&n, V &&m, int i = 5, ...); + + // Guarded with enable_if. + template <typename T> + Test2(T &&n, int i = 5, std::enable_if_t<sizeof(int) < sizeof(long), int> a = 5, ...); + + // Guarded with enable_if. + template <typename T, typename X = typename std::enable_if<sizeof(int) < sizeof(long), double>::type &> + Test2(T &&n); + + // Guarded with enable_if. + template <typename T> + Test2(T &&n, typename std::enable_if<just_true<T>>::type **a = nullptr); + + // Guarded with enable_if. + template <typename T, typename X = std::enable_if_t<just_true<T>> *&&> + Test2(T &&n, double d = 0.0); + + // Not a forwarding reference parameter. + template <typename T> + 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 <typename T> + Test2(T &&n, int x); + + // Not a forwarding reference parameter. + template <typename T> + Test2(U &&n); +}; + +// The copy and move constructors are both disabled. +class Test3 { +public: + template <typename T> + Test3(T &&n); + + template <typename T> + 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 <typename T> + 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 <typename T> + 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 <typename T> + 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); +}; |
