[libc++] Properly handle specializations of std::is_placeholder.

[• Quuxplusone]

Summary

Before this patch, the user needed to specialize both of is_placeholder<MyType> and is_placeholder<const MyType>.
After this patch, only the former is needed (although the latter is harmless if provided).

The new tests don't actually fail unless return type deduction is used, which is a C++14 feature. Specializing is_placeholder is still allowed in C++11, though.

All of the above applies to is_bind_expression, too.

Fixes https://github.com/llvm/llvm-project/issues/51095

[jloser]

LGTM % a small question FWIW.

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/is_bind_expression.pass.cpp

26

Since the library code does __uncvref_t, should we also test volatile T here and below? Or is your thought "down with volatile in the library!" basically? :)

[• Quuxplusone]

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/is_bind_expression.pass.cpp

26

Basically the latter, yeah. ;)
I mean, suppose something here didn't work properly with volatile T: I'd still call that a bug and want to fix it (and regression-test it at that point). But I have the vague impression that the paper standard might not consider that a bug — does library stuff need to work with volatile, or does volatile void the warranty in the same way as a throwing destructor? (I'm not sure.)
Plus, ideally volatile-specific bugs should be extremely rare, because most of what the library does with cv-qualifiers is colorblind as to whether it's C or V, right? Like, in this particular PR we're doing __uncvref_t which strips const and volatile symmetrically. I idealistically hope that most library code is like this, so testing const should get you most of the benefits of testing both, even if const bugs and volatile bugs were equally likely in the wild (which as you point out, they're not, because nobody should be using volatile like this). (Counterpoint: obviously the language does not treat const and volatile perfectly symmetrically: there's lots of const T& in the library and no volatile T&.)
Bottom line: Is it worth doubling the length of every test in order to preempt volatile-specific bugs? (I'm very much convinced that the answer is "no.")

[ldionne]

Do I understand correctly that we are technically conforming without this change, however this resolves implementation divergence? If so, let's create a LWG issue just to make sure the Standard actually describes what all implementations are doing.

Side note: The preferred way of referring to bugs in the code base should be http://llvm.org/PR51753. Unfortunately, that won't cause GitHub to automatically close issues when the fix is committed, which is kind of annoying. I'm on the fence as to what we should do about this, but I guess for now it's fine to use either (but let's use llvm.org/PRXXXXX inside code comments).

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/is_bind_expression.pass.cpp

26

Agreed here. We should test volatile when we have volatile-specific behavior or when we fix an actual volatile-related bug (so we add a regression test), but otherwise I'd avoid adding complexity just for that qualifier.

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/is_placeholder.pass.cpp

22–24

LIBCPP_STATIC_ASSERT instead?

Also, this should have been _LIBCPP_VERSION instead -- I guess that's one additional reason to use LIBCPP_STATIC_ASSERT.

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/specialization.pass.cpp

18

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isplace/specialization.pass.cpp

18

[• Quuxplusone]

Side note: The preferred way of referring to bugs in the code base should be http://llvm.org/PR51753.

Makes sense. I was unsure whether that would actually redirect to the proper GH issue (notice the GH number is different from the Bugzilla "PR" number), but it turns out that it does, so, cool. :)
I still plan to use Fixes #51095 in the git commit message, unless you tell me not to.

Anyway, I'm not going to land this until buildkite comes back and is green.

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/is_placeholder.pass.cpp

22–24

Yikes, good catch. I was (temporarily?) unaware that LIBCPP_STATIC_ASSERT existed; I knew LIBCPP_ASSERT but didn't think to look for the other one. Fixed.

[• Quuxplusone]

...and I forgot to answer @ldionne's question about conformingness, so I'll do it here!

My understanding is that std::bind is just massively underspecified. For example, https://eel.is/c++draft/func.bind.isbind says

The function template bind uses is_­bind_­expression to detect subexpressions. [...] A program may specialize this template for a program-defined type T to have a base characteristic of true_­type to indicate that T should be treated as a subexpression in a bind call.

But, like, how does it use is_­bind_­expression to detect subexpressions? and what does it mean to "be treated as a subexpression"? and what happens if this template is specialized to have a base characteristic of something-other-than-true_type? None of this is written out. But I think clearly the intent is what's happening in this PR: a single specialization for T should magically apply to all cv-qualifications and value-categories of T.

It is also never really specified what should happen if the user queries std::is_placeholder_v<U> for any particular U; is_placeholder is fundamentally a mechanism for customizing the library's behavior, not for querying it.

#include <functional>
struct P {};
template<> struct std::is_placeholder<P> : std::true_type {};
static_assert(std::is_placeholder_v<const P>);  // unspecified

Here's the exhaustive rundown: https://godbolt.org/z/jcTnqY6vb
There's some implementation divergence, but not as much as I originally thought.

[ldionne]

In D116388#3214312, @Quuxplusone wrote:

...and I forgot to answer @ldionne's question about conformingness, so I'll do it here!

My understanding is that std::bind is just massively underspecified. For example, https://eel.is/c++draft/func.bind.isbind says

Alright, in that case the LWG issue for fixing this would basically need to be a paper that rewrites a bunch of it. Nevermind on that.

The function template bind uses is_­bind_­expression to detect subexpressions. [...] A program may specialize this template for a program-defined type T to have a base characteristic of true_­type to indicate that T should be treated as a subexpression in a bind call.

But, like, how does it use is_­bind_­expression to detect subexpressions? and what does it mean to "be treated as a subexpression"? and what happens if this template is specialized to have a base characteristic of something-other-than-true_type? None of this is written out. But I think clearly the intent is what's happening in this PR: a single specialization for T should magically apply to all cv-qualifications and value-categories of T.

[...]

Thanks a lot for the detailed explanation. Another question on the approach, then. Why don't we do this instead?

template<class _Tp> struct is_placeholder; // forward decl only
template<class _Tp> struct __is_placeholder : integral_constant<int, 0> { };
template<class _Tp> struct __is_placeholder<_Tp const> : is_placeholder<_Tp> { };
template<class _Tp> struct __is_placeholder<_Tp volatile> : is_placeholder<_Tp> { };
template<class _Tp> struct __is_placeholder<_Tp const volatile> : is_placeholder<_Tp> { };
// etc for all cvrefs

template<class _Tp> 
struct is_placeholder : __is_placeholder<_Tp> { };

template<int _Np>
struct __is_placeholder<placeholders::__ph<_Np> > : integral_constant<int, _Np> { };

The benefit of this approach is that is_placeholder<_Tp cvref> would give the right answer for anyone querying it.

Also, if you look at the implementation of is_placeholder before this patch, it looks like we already handle const and volatile correctly. What you're fixing can only be reference qualifiers, right? If I'm right, the commit message should be adjusted.

Edit: Sorry, this was sent out while I was still editing.

[• Quuxplusone]

@ldionne: I believe your suggestion doesn't work (your __is_placeholder<const _Tp> specialization ends up unused, right?), but playing around with it got me to this version that I think works and is arguably cleaner — if much much metaprogrammier. :) WDYT?

[• Quuxplusone]

@ldionne ping!

[ldionne]

I think that works and is indeed nicer. LGTM with uglification.

libcxx/include/__functional/bind.h

28

We need to uglify this as _Tp (applies below too).

libcxx/include/__functional/bind.h

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template<class _Tp> struct __is_bind_expression : public false_type {};
 template<class _Tp> struct _LIBCPP_TEMPLATE_VIS is_bind_expression
     : public __is_bind_expression<typename remove_cv<_Tp>::type> {};
 
 #if _LIBCPP_STD_VER > 14
 template <class _Tp>
 inline constexpr size_t is_bind_expression_v = is_bind_expression<_Tp>::value;
 #endif
 
 template<class _Tp> struct __is_placeholder : public integral_constant<int, 0> {};
 template<class _Tp> struct _LIBCPP_TEMPLATE_VIS is_placeholder

 {
     return __ti(_VSTD::forward<_Uj>(_VSTD::get<_Indx>(__uj))...);
 }
 
 template <class _Ti, class ..._Uj>
 inline _LIBCPP_INLINE_VISIBILITY
 typename __enable_if_t
 <
-    is_bind_expression<_Ti>::value,
+    is_bind_expression<__uncvref_t<_Ti>>::value,
     __invoke_of<_Ti&, _Uj...>
 >::type
 __mu(_Ti& __ti, tuple<_Uj...>& __uj)
 {
     typedef typename __make_tuple_indices<sizeof...(_Uj)>::type __indices;
     return _VSTD::__mu_expand(__ti, __uj, __indices());
 }
 
 template <bool IsPh, class _Ti, class _Uj>
 struct __mu_return2 {};
 
 template <class _Ti, class _Uj>
 struct __mu_return2<true, _Ti, _Uj>
 {
-    typedef typename tuple_element<is_placeholder<_Ti>::value - 1, _Uj>::type type;
+    typedef typename tuple_element<is_placeholder<__uncvref_t<_Ti>>::value - 1, _Uj>::type type;
 };
 
 template <class _Ti, class _Uj>
 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
-    0 < is_placeholder<_Ti>::value,
-    typename __mu_return2<0 < is_placeholder<_Ti>::value, _Ti, _Uj>::type
+    0 < is_placeholder<__uncvref_t<_Ti>>::value,
+    typename __mu_return2<0 < is_placeholder<__uncvref_t<_Ti>>::value, _Ti, _Uj>::type
 >::type
 __mu(_Ti&, _Uj& __uj)
 {
-    const size_t _Indx = is_placeholder<_Ti>::value - 1;
+    const size_t _Indx = is_placeholder<__uncvref_t<_Ti>>::value - 1;
     return _VSTD::forward<typename tuple_element<_Indx, _Uj>::type>(_VSTD::get<_Indx>(__uj));
 }
 
 template <class _Ti, class _Uj>
 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
-    !is_bind_expression<_Ti>::value &&
-    is_placeholder<_Ti>::value == 0 &&
+    !is_bind_expression<__uncvref_t<_Ti>>::value &&
+    is_placeholder<__uncvref_t<_Ti>>::value == 0 &&
     !__is_reference_wrapper<_Ti>::value,
     _Ti&
 >::type
 __mu(_Ti& __ti, _Uj&)
 {
     return __ti;
 }
 

 struct __mu_return_impl<_Ti, false, true, false, tuple<_Uj...> >
     : public __mu_return_invokable<__invokable<_Ti&, _Uj...>::value, _Ti, _Uj...>
 {
 };
 
 template <class _Ti, class _TupleUj>
 struct __mu_return_impl<_Ti, false, false, true, _TupleUj>
 {
-    typedef typename tuple_element<is_placeholder<_Ti>::value - 1,
+    typedef typename tuple_element<is_placeholder<__uncvref_t<_Ti>>::value - 1,
                                    _TupleUj>::type&& type;
 };
 
 template <class _Ti, class _TupleUj>
 struct __mu_return_impl<_Ti, true, false, false, _TupleUj>
 {
     typedef typename _Ti::type& type;
 };
 
 template <class _Ti, class _TupleUj>
 struct __mu_return_impl<_Ti, false, false, false, _TupleUj>
 {
     typedef _Ti& type;
 };
 
 template <class _Ti, class _TupleUj>
 struct __mu_return
     : public __mu_return_impl<_Ti,
                               __is_reference_wrapper<_Ti>::value,
-                              is_bind_expression<_Ti>::value,
-                              0 < is_placeholder<_Ti>::value &&
-                              is_placeholder<_Ti>::value <= tuple_size<_TupleUj>::value,
+                              is_bind_expression<__uncvref_t<_Ti>>::value,
+                              0 < is_placeholder<__uncvref_t<_Ti>>::value &&
+                              is_placeholder<__uncvref_t<_Ti>>::value <= tuple_size<_TupleUj>::value,
                               _TupleUj>
 {
 };
 
 template <class _Fp, class _BoundArgs, class _TupleUj>
 struct __is_valid_bind_return
 {
     static const bool value = false;

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/is_bind_expression.pass.cpp

 #include <functional>
 #include "test_macros.h"
 
 template <bool Expected, class T>
 void
 test(const T&)
 {
     static_assert(std::is_bind_expression<T>::value == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_bind_expression<T&>::value == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_bind_expression<const T>::value == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_bind_expression<const T&>::value == Expected, "");
+
 #if TEST_STD_VER > 14
     static_assert(std::is_bind_expression_v<T> == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_bind_expression_v<T&> == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_bind_expression_v<const T> == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_bind_expression_v<const T&> == Expected, "");
 #endif
 }
 
 struct C {int operator()(...) const { return 0; }};
 
 int main(int, char**)
 {
     test<true>(std::bind(C()));
     test<true>(std::bind(C(), std::placeholders::_2));
     test<true>(std::bind<int>(C()));
     test<false>(1);
     test<false>(std::placeholders::_2);
 
   return 0;
 }

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isbind/specialization.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11
+
+// <functional>
+
+// template<class T> struct is_bind_expression;
+//   A program may specialize this template for a program-defined type T
+//   to have a base characteristic of true_type to indicate that T should
+//   be treated as a subexpression in a bind call.
+//   https://llvm.org/PR51753
+
+#include <functional>
+#include <cassert>
+#include <type_traits>
+
+struct MyBind {
+    int operator()(int x, int y) const { return 10*x + y; }
+};
+template<> struct std::is_bind_expression<MyBind> : std::true_type {};
+
+int main(int, char**)
+{
+  {
+    auto f = [](auto x) { return 10*x + 9; };
+    MyBind bindexpr;
+    auto bound = std::bind(f, bindexpr);
+    assert(bound(7, 8) == 789);
+  }
+  {
+    auto f = [](auto x) { return 10*x + 9; };
+    const MyBind bindexpr;
+    auto bound = std::bind(f, bindexpr);
+    assert(bound(7, 8) == 789);
+  }
+  {
+    auto f = [](auto x) { return 10*x + 9; };
+    MyBind bindexpr;
+    auto bound = std::bind(f, std::move(bindexpr));
+    assert(bound(7, 8) == 789);
+  }
+  {
+    auto f = [](auto x) { return 10*x + 9; };
+    const MyBind bindexpr;
+    auto bound = std::bind(f, std::move(bindexpr));
+    assert(bound(7, 8) == 789);
+  }
+
+  return 0;
+}

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isplace/is_placeholder.pass.cpp

 #include <functional>
 #include "test_macros.h"
 
 template <int Expected, class T>
 void
 test(const T&)
 {
     static_assert(std::is_placeholder<T>::value == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_placeholder<T&>::value == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_placeholder<const T>::value == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_placeholder<const T&>::value == Expected, "");
+
 #if TEST_STD_VER > 14
     static_assert(std::is_placeholder_v<T> == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_placeholder_v<T&> == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_placeholder_v<const T> == Expected, "");
+    LIBCPP_STATIC_ASSERT(std::is_placeholder_v<const T&> == Expected, "");
 #endif
 }
 
 struct C {};
 
 int main(int, char**)
 {
     test<1>(std::placeholders::_1);

libcxx/test/std/utilities/function.objects/bind/func.bind/func.bind.isplace/specialization.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11
+
+// <functional>
+
+// template<class T> struct is_placeholder;
+//   A program may specialize this template for a program-defined type T
+//   to have a base characteristic of integral_constant<int, N> with N > 0
+//   to indicate that T should be treated as a placeholder type.
+//   https://llvm.org/PR51753
+
+#include <functional>
+#include <cassert>
+#include <type_traits>
+
+struct My2 {};
+template<> struct std::is_placeholder<My2> : std::integral_constant<int, 2> {};
+
+int main(int, char**)
+{
+  {
+    auto f = [](auto x) { return 10*x + 9; };
+    My2 place;
+    auto bound = std::bind(f, place);
+    assert(bound(7, 8) == 89);
+  }
+  {
+    auto f = [](auto x) { return 10*x + 9; };
+    const My2 place;
+    auto bound = std::bind(f, place);
+    assert(bound(7, 8) == 89);
+  }
+  {
+    auto f = [](auto x) { return 10*x + 9; };
+    My2 place;
+    auto bound = std::bind(f, std::move(place));
+    assert(bound(7, 8) == 89);
+  }
+  {
+    auto f = [](auto x) { return 10*x + 9; };
+    const My2 place;
+    auto bound = std::bind(f, std::move(place));
+    assert(bound(7, 8) == 89);
+  }
+
+  return 0;
+}