[clang-tidy] Forwarding reference overload in constructors

[leanil]

Summary

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.

[leanil]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

22 ↗

(On Diff #90367)

This check could probably be simpler, but I wanted to be more effective than just using QualType.getAsString().

[JonasToth]

please add the check in the release notes as well.

[JonasToth]

added my thoughts. i like the check, seems really thought out :)

docs/clang-tidy/checks/misc-forwarding-reference-overload.rst

36 ↗

(On Diff #90367)

maybe "can act like described above" should be different.
i think being more explicit with the hiding/unexpected call to those is clearer.

test/clang-tidy/misc-forwarding-reference-overload.cpp

21 ↗

(On Diff #90367)

could the check output a note when there was a userdefined constructor and point to that one if it gets hidden? that would make it clearer, what and how the problem occurs.

28 ↗

(On Diff #90367)

maybe be more explicit with whats Ok with a comment // Ok or sth like that on all following constructors?

[aaron.ballman]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

23 ↗

(On Diff #90367)

This should be CheckTemplate per our usual coding conventions.

39 ↗

(On Diff #90367)

Please use auto * (or const auto `).

44 ↗

(On Diff #90367)

Same here.

119 ↗

(On Diff #90367)

I think this can be lowered into the for loop as auto Iter = Ctor->param_begin() + 1.

[leanil]

Added changes according to the comments.

[leanil]

test/clang-tidy/misc-forwarding-reference-overload.cpp

21 ↗

(On Diff #90367)

What do you mean by "if it gets hidden"? Should I look for specific calls that are hijacked by the perfect forwarding ctor? Or just make a note on the user defined copy/move ctors any time I produce a warning (without looking at actual calls)?
I think the former would be quite tricky to do.
Also, what if the perfect forwarding ctor hides the compiler generated copy/move? Should I still make a note (maybe pointing to the class itself)?

[JonasToth]

docs/clang-tidy/checks/misc-forwarding-reference-overload.rst

38 ↗

(On Diff #90569)

typo: prefect -> perfect

test/clang-tidy/misc-forwarding-reference-overload.cpp

21 ↗

(On Diff #90367)

class Person {
public:
  // perfect forwarding ctor
  template<typename T>
  explicit Person(T&& n) {}

  // (possibly compiler generated) copy ctor
  Person(const Person& rhs); 
};

a note pointing to a user defined ctor would be good i think.
with compiler generated operations the warning is clear.

emitting such a note would be a minor feature and if too complicated not worth it, i guess.

[aaron.ballman]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

31 ↗

(On Diff #90569)

This should be using equals() rather than find() -- otherwise a name like enable_if_awesome() would trigger the match. You should also make sure the name is in the ::std namespace so that you can't trigger it with foo::enable_if or foo::std::enable_if.

35 ↗

(On Diff #90569)

Reference types as well? Or is that not sensible?

53 ↗

(On Diff #90569)

This seems generally useful and should probably be in ASTMatchers.h. However, that can be done in a follow-up patch.

118 ↗

(On Diff #90569)

I don't think this if statement is required -- Ctor should always be found, no?

125–126 ↗

(On Diff #90569)

I think a better diagnostic might be: "constructor accepting a universal reference hides the %select{copy|move|both the copy and move}0 %select{constructor{|s}1"

And then provide a note ("%select{copy|move}0 constructor declared here") that points to the offending copy and/or move constructor.

clang-tidy/misc/MiscTidyModule.cpp

70 ↗

(On Diff #90569)

I wonder if the name might be slightly misleading -- I've always understood these to be universal references rather than forwarding references. I don't have the Meyers book in front of me -- what nomenclature does he use?

Once we pick the name, we should use it consistently in the source code (like the file name for the check and the check name), the documentation, and the release notes.

[leanil]

Fix enable_if detection in pointer and reference types.
Improve test coverage for these cases.

[leanil]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

31 ↗

(On Diff #90569)

My intention with using find() was that if a user has an enable_if_awesome(), then it seems more likely to be similar to the std version, than to have completely different purpose.
And as a bonus find() also finds std::enable_if_t in one check.

125–126 ↗

(On Diff #90569)

Without checking actual constructor calls, I would have to make notes on every (non disabled) copy / move constructor, any time I produce a warning. And as the warning already states where the problem lies, the notes would only help people find the copy and move constructors. Do you think that's necessary?

clang-tidy/misc/MiscTidyModule.cpp

70 ↗

(On Diff #90569)

Meyers calls them universal references, but it's forwarding reference in the standard (14.8.2.1).

[aaron.ballman]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

31 ↗

(On Diff #90569)

We don't typically use a heuristic approach like that -- instead, we often let the user specify their own list of names explicitly, as an option.

125–126 ↗

(On Diff #90569)

The warning states where the forwarding reference constructor is, but it doesn't state where the conflicting constructors are. When we issue diagnostics like that, we generally use notes so that the user can see all of the locations involved -- the user may want to get rid of the other constructors, or they may want to get rid of the forwarding reference constructor. Also, saying "can hide" implies that it isn't hiding anything at all, just that it's possible to do so. Tightening up the wording and showing all of the locations involved solves both problems.

clang-tidy/misc/MiscTidyModule.cpp

70 ↗

(On Diff #90569)

Hmm, the terms are a bit too new to really get a good idea from google's ngram viewer, but the search result counts are:

Google:
"universal reference" : 270,000
"forwarding reference" : 9650

Stack Overflow:
universal reference : 3016
forwarding reference: 1654

So I think that these are probably more well-known as universal references, despite the standard's nomenclature being forwarding reference.

[malcolm.parsons]

clang-tidy/misc/MiscTidyModule.cpp

70 ↗

(On Diff #90569)

The Q&A section in https://isocpp.org/files/papers/N4164.pdf explains why "universal reference" is a bad name.

[xazax.hun]

clang-tidy/misc/MiscTidyModule.cpp

70 ↗

(On Diff #90569)

I think in a compiler related project at least in the source code we should stick the the standard's nomenclature. In the documentation, however, it is worth to mention that, this very same thing is called universal reference by Scott Meyers.

[aaron.ballman]

clang-tidy/misc/MiscTidyModule.cpp

70 ↗

(On Diff #90569)

Okay, I'm convinced enough that "forward reference" is okay. :-)

[leanil]

Suppress warning only on std::enable_if.
Make note on copy and move constructors.

[aaron.ballman]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

125–126 ↗

(On Diff #90569)

This isn't quite complete. It's still an ambiguous statement to say "it can hide"; it does hide these constructors, and we even know which ones. Emit the notes before you emit the main diagnostic and you can use the %select suggested originally to be specific in the diagnostic.

127–128 ↗

(On Diff #92542)

You can use a range-based for loop here instead.

129–137 ↗

(On Diff #92542)

This can be simplified by using a %select as well.

docs/clang-tidy/checks/misc-forwarding-reference-overload.rst

48 ↗

(On Diff #92542)

This is no longer accurate.

[xazax.hun]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

125–126 ↗

(On Diff #90569)

We can not say for sure without looking at a concrete call whether a constructor is "hidden" or not. It is always determined during the overload resolution.

This check does not consider the calls, because that way it would always miss the possible misuses if libraries.

[aaron.ballman]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

125–126 ↗

(On Diff #90569)

I can see the logic in that. I guess I'm thinking of it the same way we use the phrase "hidden" when describing code like:

struct Base {
  virtual void f(int);
};

struct Derived : Base {
  void f(double);
};

We claim Derived::f() hides Base::f() without considering the callers.

[xazax.hun]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

125–126 ↗

(On Diff #90569)

I see. In that case maybe we should come up with a less confusing term like hijacking overload? The constructors are still part of the overload set, so no hiding as in the standard's nomenclature happens here, but the overload resolution is not doing what the user would expect in these cases.

[aaron.ballman]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

125–126 ↗

(On Diff #90569)

I'm also fine going back to being somewhat more wishy-washy in our phrasing (can hide).

What do you think about using the %select to specify what can be hidden, rather than always talking about copy and move constructors (one of which may not even be present in the user's source)?

[leanil]

Simplify the note generation and correct the documentation.

[leanil]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

125–126 ↗

(On Diff #90569)

I think the (assumed) use of a compiler generated copy or move is similarly problematic as in the case of a user defined one, so I would keep mentioning both.

[aaron.ballman]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

127 ↗

(On Diff #92765)

Should be const auto * rather than just auto.

131 ↗

(On Diff #92765)

This passes in too many arguments to the diagnostic, I think OtherCtor should be removed.

125–126 ↗

(On Diff #90569)

That would be handled automatically anyway (the select I suggested has an option for both). Also, you should have a test for the case involving the implicitly declared constructors.

[leanil]

Make the warning message more precise.
Add tests for implicitly declared constructors.

[JonasToth]

Why does diff contain so many files?
Could you maybe merge the latest master into your branch?

[leanil]

Correct earlier diff issue with outdated master.

[leanil]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

119 ↗

(On Diff #93114)

This is the most precise way to formulate the warning message I could come up with.
The condition for excluding either "copy" or "move" from the warning is to find only disabled instances of the constructor type, and there must be at least one, otherwise the compiler generated constructor (which is not present in this enumeration) can be hidden.

[aaron.ballman]

clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

119 ↗

(On Diff #93114)

How about:

if (OtherCtor->isDeleted() || OtherCtor->getAccess() == AS_private)
  (OtherCtor->isCopyConstructor() ? DisabledCopy : DisabledMove) = true;
else
  (OtherCtor->isCopyConstructor() ? EnabledCopy : EnabledMove) = true;

136 ↗

(On Diff #93114)

Can elide the braces.

140 ↗

(On Diff #93114)

copy and the move -> copy and move

142 ↗

(On Diff #93114)

The math is correct, but really makes me scratch my head to try to puzzle through it. Why not something along the lines of Copy && Move ? 2 : (Copy ? 0 : 1)

[leanil]

Simplify checking the presence of copy and move ctors.

[aaron.ballman]

Aside from some minor nits about newlines, LGTM!

docs/clang-tidy/checks/misc-forwarding-reference-overload.rst

51–52 ↗

(On Diff #93774)

You can remove the empty lines at the end.

test/clang-tidy/misc-forwarding-reference-overload.cpp

140 ↗

(On Diff #93774)

Please add a newline at the end of the file.

[leanil]

fix new lines

[aaron.ballman]

LGTM! If you need someone to commit on your behalf, let us know. I won't be able to do it this week. but I'm guessing @alexfh or someone else may be available to help (or I can get to it next week).

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
+  ForwardingReferenceOverloadCheck.cpp
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.h

+//===--- 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

clang-tools-extra/trunk/clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp

+//===--- 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

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
+#include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedConstCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"

 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     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");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<DanglingHandleCheck>("misc-dangling-handle");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `hicpp` module
   Adds checks that implement the `High Integrity C++ Coding Standard <http://www.codingstandard.com/section/index/>`_ and other safety
   standards. Many checks are aliased to other modules.
 
 - New `modernize-return-braced-init-list
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-return-braced-init-list.html>`_ check
 
   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).
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-bool-pointer-implicit-conversion
    misc-dangling-handle
    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
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-const
    misc-misplaced-widening-cast
    misc-move-const-arg

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forwarding-reference-overload.rst

+.. 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.

clang-tools-extra/trunk/test/clang-tidy/misc-forwarding-reference-overload.cpp

+// 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);
+};