[clang-tools-extra] 68546c9 - bugprone-forwarding-reference-overload: support non-type template parameters
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 29 04:01:58 PDT 2021
Author: Jesse Towner
Date: 2021-07-29T07:01:19-04:00
New Revision: 68546c9d6fc54a7ca7ad487cd4b6a5dafea9b4f3
URL: https://github.com/llvm/llvm-project/commit/68546c9d6fc54a7ca7ad487cd4b6a5dafea9b4f3
DIFF: https://github.com/llvm/llvm-project/commit/68546c9d6fc54a7ca7ad487cd4b6a5dafea9b4f3.diff
LOG: bugprone-forwarding-reference-overload: support non-type template parameters
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.
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
clang-tools-extra/docs/clang-tidy/checks/bugprone-forwarding-reference-overload.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
index aeb3db92f16ec..857ef15f9d0c7 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
@@ -74,9 +74,15 @@ void ForwardingReferenceOverloadCheck::registerMatchers(MatchFinder *Finder) {
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);
}
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-forwarding-reference-overload.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-forwarding-reference-overload.rst
index b2a9e0f3b3dfb..37078d498b330 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-forwarding-reference-overload.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-forwarding-reference-overload.rst
@@ -26,9 +26,14 @@ Consider the following example:
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);
};
@@ -37,13 +42,15 @@ 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.
+
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp
index e5d0ba395d143..920029b56e55d 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp
@@ -150,3 +150,93 @@ class variant {
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;
+};
More information about the cfe-commits
mailing list