[clang-tools-extra] r299638 - [clang-tidy] Check for forwarding reference overload in constructors.

Gabor Horvath via cfe-commits cfe-commits at lists.llvm.org
Thu Apr 6 02:56:42 PDT 2017


Author: xazax
Date: Thu Apr  6 04:56:42 2017
New Revision: 299638

URL: http://llvm.org/viewvc/llvm-project?rev=299638&view=rev
Log:
[clang-tidy] Check for forwarding reference overload in constructors.

Patch by AndrĂ¡s Leitereg!

Differential Revision: https://reviews.llvm.org/D30547

Added:
    clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp
    clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.h
    clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst
    clang-tools-extra/trunk/test/clang-tidy/misc-forwarding-reference-overload.cpp
Modified:
    clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
    clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
    clang-tools-extra/trunk/docs/ReleaseNotes.rst
    clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt?rev=299638&r1=299637&r2=299638&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt Thu Apr  6 04:56:42 2017
@@ -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

Added: clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp?rev=299638&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp Thu Apr  6 04:56:42 2017
@@ -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

Added: clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.h?rev=299638&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.h Thu Apr  6 04:56:42 2017
@@ -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

Modified: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp?rev=299638&r1=299637&r2=299638&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp Thu Apr  6 04:56:42 2017
@@ -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");

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=299638&r1=299637&r2=299638&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Thu Apr  6 04:56:42 2017
@@ -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).

Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=299638&r1=299637&r2=299638&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Thu Apr  6 04:56:42 2017
@@ -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

Added: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst?rev=299638&view=auto
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst (added)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst Thu Apr  6 04:56:42 2017
@@ -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.

Added: clang-tools-extra/trunk/test/clang-tidy/misc-forwarding-reference-overload.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-forwarding-reference-overload.cpp?rev=299638&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/misc-forwarding-reference-overload.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/misc-forwarding-reference-overload.cpp Thu Apr  6 04:56:42 2017
@@ -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);
+};




More information about the cfe-commits mailing list