[clang-tidy] bugprone-forwarding-reference-overload: support non-type template parameters

[jwtowner]

Summary

Many concepts emulation libraries, such as the one found in Range v3, tend to
use non-type template parameters for the enable_if type expression, due to
their versatility in template functions and constructors containing variadic
template parameter packs.

Unfortunately the bugprone-forwarding-reference-overload check does not
handle non-type template parameters, as was first noted in this bug report:
https://bugs.llvm.org/show_bug.cgi?id=38081

This patch fixes this long standing issue and allows for the check to be suppressed
with the use of a non-type template parameter containing enable_if or enable_if_t in
the type expression, so long as it has a default literal value.

[jwtowner]

clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp

83

Note that I'm using hasDescendant here, because sometimes the literal ends up inside of an ImplicitCastExpr node and sometimes its a direct child of the NonTypeTemplateParmDecl node. I'm not sure exactly what causes the difference.

[jwtowner]

Friendly bump in case this was missed. Without this fix, bugprone-forwarding-reference-overload is not currently usable in a lot of codebases.

[aaron.ballman]

Thank you for the ping, this fell off my radar. LGTM aside from some small nits.

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

34

57

Can you add the newline back?

[jwtowner]

Improved the code formatting of the samples in the documentation and added back in the newline at the end of the bugprone-forwarding-reference-overload.rst

[jwtowner]

It should be good to go. I do not have commit access so feel free to commit. Thanks!

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

34

The reason I didn't put the spaces in here was that I was trying to match the style above for enable_if_t<is_special<T>,void>. I can add a space after the comma there as well.

57

Will do! Sorry about this.

[aaron.ballman]

In D103385#2900967, @jwtowner wrote:

It should be good to go. I do not have commit access so feel free to commit. Thanks!

Thanks! What name and email address would you like me to use for patch attribution?

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

34

Ah, good call. The way it is now is good for me. Thank you!

[jwtowner]

Thanks! What name and email address would you like me to use for patch attribution?

The following will do:

Jesse Towner <townerj AT gmail.com>

[aaron.ballman]

I've committed on your behalf in 68546c9d6fc54a7ca7ad487cd4b6a5dafea9b4f3, thank you for the fix!

clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp

auto ForwardingRefParm =
           .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(
+          unless(hasParent(functionTemplateDecl(anyOf(
               // No warning: enable_if as type parameter.
-              hasDefaultArgument(isEnableIf())))))))
+              has(templateTypeParmDecl(hasDefaultArgument(isEnableIf()))),
+              // No warning: enable_if as non-type template parameter.
+              has(nonTypeTemplateParmDecl(
+                  hasType(isEnableIf()),
+                  anyOf(hasDescendant(cxxBoolLiteral()),
+                        hasDescendant(cxxNullPtrLiteralExpr()),
+                        hasDescendant(integerLiteral())))))))))
           .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 =

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

public:
       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>>
+      template<typename T, typename X = enable_if_t<is_special<T>, void>>
       explicit Person(T&& n) {}
 
+      // C4: variadic perfect forwarding ctor guarded with enable_if
+      template<typename... A,
+        enable_if_t<is_constructible_v<tuple<string, int>, A&&...>, int> = 0>
+      explicit Person(A&&... a) {}
+
       // (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.
+like C3 and C4 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 constructor
-parameters. If any part of these types is ``std::enable_if`` or ``std::enable_if_t``,
-we assume the constructor is guarded.
+types of the constructor parameters, the default values of template type parameters
+and the types of non-type template parameters with a default literal value. If any
+part of these types is ``std::enable_if`` or ``std::enable_if_t``, we assume the
+constructor is guarded.
+

clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp

 
 template <class... _Types>
 class variant {
 public:
   template <class _Arg, class _Tp = __best_match_t<_Arg, _Types...> >
   constexpr variant(_Arg&& __arg) {}
   // CHECK-NOTES: :[[@LINE-1]]:13: warning: constructor accepting a forwarding reference can hide the copy and move constructors
 };
+
+namespace std {
+template <class T, class U> struct is_same { static constexpr bool value = false; };
+template <class T> struct is_same<T, T> { static constexpr bool value = true; };
+template <class T, class U> constexpr bool is_same_v = is_same<T, U>::value;
+template <class T> struct remove_reference { using type = T; };
+template <class T> struct remove_reference<T&> { using type = T; };
+template <class T> struct remove_reference<T&&> { using type = T; };
+template <class T> using remove_reference_t = typename remove_reference<T>::type;
+template <class T> struct remove_cv { using type = T; };
+template <class T> struct remove_cv<const T> { using type = T; };
+template <class T> struct remove_cv<volatile T> { using type = T; };
+template <class T> struct remove_cv<const volatile T> { using type = T; };
+template <class T> using remove_cv_t = typename remove_cv<T>::type;
+template <class T> struct remove_cvref { using type = remove_cv_t<remove_reference_t<T>>; };
+template <class T> using remove_cvref_t = typename remove_cvref<T>::type;
+} // namespace std
+
+// Handle enable_if when used as a non-type template parameter.
+class Test7 {
+public:
+  // Guarded with enable_if.
+  template <class T,
+    typename std::enable_if_t<std::is_same_v<std::remove_cvref_t<T>, int>, int>::type = 0>
+  Test7(T &&t);
+
+  // Guarded with enable_if.
+  template <class T,
+    std::enable_if_t<
+      !std::is_same_v<std::remove_cvref_t<T>, Test7>
+      && !std::is_same_v<std::remove_cvref_t<T>, bool>, int> = true>
+  Test7(T &&t);
+
+  Test7(const Test7 &other) = default;
+  Test7(Test7 &&other) = default;
+};
+
+// Handle enable_if when used as a non-type template parameter following
+// a variadic template parameter pack.
+class Test8 {
+public:
+  // Guarded with enable_if.
+  template <class T, class... A,
+    std::enable_if_t<
+      !std::is_same_v<std::remove_cvref_t<T>, Test8>
+      || (sizeof...(A) > 0)>* = nullptr>
+  Test8(T &&t, A &&... a);
+
+  Test8(const Test8 &other) = default;
+  Test8(Test8 &&other) = default;
+};
+
+// Non-type template parameter failure cases.
+class Test9 {
+public:
+  // Requires a default argument (such as a literal, implicit cast expression, etc.)
+  template <class T,
+    std::enable_if_t<std::is_same_v<std::remove_cvref_t<T>, bool>, int>>
+  Test9(T &&t);
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors
+  // CHECK-NOTES: 240:3: note: copy constructor declared here
+  // CHECK-NOTES: 241:3: note: move constructor declared here
+
+  // Requires a default argument (such as a literal, implicit cast expression, etc.)
+  template <class T,
+    std::enable_if_t<std::is_same_v<std::remove_cvref_t<T>, long>>*>
+  Test9(T &&t);
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors
+  // CHECK-NOTES: 240:3: note: copy constructor declared here
+  // CHECK-NOTES: 241:3: note: move constructor declared here
+
+  // Only std::enable_if or std::enable_if_t are supported
+  template <class T,
+    typename std::enable_if_nice<T>::type* = nullptr>
+  Test9(T &&t);
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors
+  // CHECK-NOTES: 240:3: note: copy constructor declared here
+  // CHECK-NOTES: 241:3: note: move constructor declared here
+
+  // Only std::enable_if or std::enable_if_t are supported
+  template <class T,
+    typename foo::enable_if<T>::type = 0>
+  Test9(T &&t);
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: constructor accepting a forwarding reference can hide the copy and move constructors
+  // CHECK-NOTES: 240:3: note: copy constructor declared here
+  // CHECK-NOTES: 241:3: note: move constructor declared here
+
+  Test9(const Test9 &other) = default;
+  Test9(Test9 &&other) = default;
+};