leanil created this revision.
leanil added a project: clang-tools-extra.
Herald added subscribers: JDevlieghere, mgorny.
Perfect forwarding constructors are called instead of copy constructors if the
forwarding reference provides a closer match (e.g. with non-const parameter).
This can be confusing to developers unfamiliar with this feature.
class Person {
public:
// perfect forwarding ctor
template<typename T>
explicit Person(T&& n) {}
// (possibly compiler generated) copy ctor
Person(const Person& rhs);
};
For detailed explanation of the issue, see Scott Meyers' Effective Modern C++,
Item 26.
Running the checker on the LLVM source produces warnings for the following
constructors:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/ADT/iterator.h#L287
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/ADT/iterator_range.h#L39
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/Allocator.h#L150
These are probably not errors, but the described issue does occur (i.e. the
perfect forwarding constructor can be called instead of the default copy), so
it may worth a double-check.
https://reviews.llvm.org/D30547
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp
clang-tidy/misc/ForwardingReferenceOverloadCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-forwarding-reference-overload.rst
test/clang-tidy/misc-forwarding-reference-overload.cpp
Index: test/clang-tidy/misc-forwarding-reference-overload.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/misc-forwarding-reference-overload.cpp
@@ -0,0 +1,70 @@
+// 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 <typename T>
+constexpr bool just_true = true;
+
+class Person {
+public:
+ template <typename T>
+ Person(T &&n);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'Person' can hide copy and move constructors [misc-forwarding-reference-overload]
+
+ template <typename T>
+ Person(T &&n, int I = 5, ...);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'Person' can hide copy and move constructors [misc-forwarding-reference-overload]
+};
+
+template <typename U>
+class Person2 {
+public:
+ template <typename T, class V>
+ Person2(T &&n, V &&m, int i = 5, ...);
+
+ template <typename T>
+ Person2(T &&n, int i = 5, std::enable_if_t<sizeof(int) < sizeof(long), int> a = 5, ...);
+
+ template <typename T, typename X = typename std::enable_if<sizeof(int) < sizeof(long), double>::type>
+ Person2(T &&n);
+
+ template <typename T>
+ Person2(T &&n, std::enable_if_t<just_true<T>, int> a = 1);
+
+ template <typename T, typename X = typename std::enable_if<just_true<T>, int>::type *>
+ Person2(T &&n, double d = 0.0);
+
+ template <typename T>
+ Person2(const T &&n);
+
+ Person2(int &&x);
+
+ template <typename T>
+ Person2(T &&n, int x);
+
+ template <typename T>
+ Person2(U &&n);
+};
+
+class Person3 {
+public:
+ template <typename T>
+ Person3(T &&n);
+
+ template <typename T>
+ Person3(T &&n, int I = 5, ...);
+
+ Person3(const Person3 &rhs) = delete;
+
+private:
+ Person3(Person3 &&rhs);
+};
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<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 act like
+described above. We suppress warnings if the copy and move constructor is
+disabled (deleted or private), because there is nothing the prefect 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 contains "enable_if" in its name, we
+assume the constructor is guarded.
+
+
+
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -64,6 +64,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: 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<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");
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,132 @@
+//===--- 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"
+
+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;
+ }
+ return Spec->getTemplateName()
+ .getAsTemplateDecl()
+ ->getTemplatedDecl()
+ ->getName()
+ .find("enable_if") != StringRef::npos;
+ };
+ const Type *BaseType = Node.getTypePtr();
+ // Case: pointer to enable_if.
+ if (BaseType->isPointerType()) {
+ BaseType = BaseType->getPointeeType().getTypePtr();
+ }
+ // Case: type parameter dependent (enable_if<is_integral<T>>).
+ if (auto Dependent = BaseType->getAs<DependentNameType>()) {
+ BaseType = Dependent->getQualifier()->getAsType();
+ }
+ if (checkTemplate(BaseType->getAs<TemplateSpecializationType>())) {
+ return true; // Case: enable_if_t< >.
+ } else if (auto Elaborated = BaseType->getAs<ElaboratedType>()) {
+ if (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");
+
+ auto HasDisabledCopy = hasMethod(
+ cxxConstructorDecl(isCopyConstructor(), anyOf(isDeleted(), isPrivate())));
+ auto HasDisabledMove = hasMethod(
+ cxxConstructorDecl(isMoveConstructor(), anyOf(isDeleted(), isPrivate())));
+
+ 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())))))),
+ unless(ofClass(
+ // No warning: disabled copy and move constructor.
+ cxxRecordDecl(HasDisabledCopy, HasDisabledMove))))
+ .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.
+ if (const auto *Ctor = Result.Nodes.getNodeAs<FunctionDecl>("ctor")) {
+ auto Iter = Ctor->param_begin();
+ for (++Iter; Iter != Ctor->param_end(); ++Iter) {
+ if (!(*Iter)->hasDefaultArg()) {
+ return;
+ }
+ }
+ diag(Ctor->getLocation(), "function %0 can hide copy and move constructors")
+ << Ctor;
+ }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
