jwtowner created this revision.
jwtowner added reviewers: alexfh, aaron.ballman, hokein.
Herald added a subscriber: xazax.hun.
jwtowner requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D103385
Files:
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
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-forwarding-reference-overload.cpp
@@ -150,3 +150,93 @@
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;
+};
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-forwarding-reference-overload.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/bugprone-forwarding-reference-overload.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-forwarding-reference-overload.rst
@@ -29,6 +29,11 @@
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,14 @@
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.
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
@@ -74,9 +74,15 @@
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);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
