[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

26–31

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

libcxx/include/__functional/bind.h

 #include <type_traits>
 
 #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> {};
+template<class _Tp>
+struct is_bind_expression : _If<
+    _IsSame<_Tp, typename __uncvref<_Tp>::type>::value,
+    false_type,
+    is_bind_expression<typename __uncvref<_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
-    : public __is_placeholder<typename remove_cv<_Tp>::type> {};
+template<class _Tp>
+struct is_placeholder : _If<
+    _IsSame<_Tp, typename __uncvref<_Tp>::type>::value,
+    integral_constant<int, 0>,
+    is_placeholder<typename __uncvref<_Tp>::type>
+> {};
 
 #if _LIBCPP_STD_VER > 14
 template <class _Tp>
 inline constexpr size_t is_placeholder_v = is_placeholder<_Tp>::value;
 #endif
 
 namespace placeholders
 {

 /* inline */ constexpr __ph<8>   _8{};
 /* inline */ constexpr __ph<9>   _9{};
 /* inline */ constexpr __ph<10> _10{};
 #endif // defined(_LIBCPP_CXX03_LANG) || defined(_LIBCPP_BUILDING_LIBRARY)
 
 } // namespace placeholders
 
 template<int _Np>
-struct __is_placeholder<placeholders::__ph<_Np> >
+struct is_placeholder<placeholders::__ph<_Np> >
     : public integral_constant<int, _Np> {};
 
 
 #ifndef _LIBCPP_CXX03_LANG
 
 template <class _Tp, class _Uj>
 inline _LIBCPP_INLINE_VISIBILITY
 _Tp&
template <class ..._Args>
         operator()(_Args&& ...__args) const
         {
             return _VSTD::__apply_functor(__f_, __bound_args_, __indices(),
                                    tuple<_Args&&...>(_VSTD::forward<_Args>(__args)...));
         }
 };
 
 template<class _Fp, class ..._BoundArgs>
-struct __is_bind_expression<__bind<_Fp, _BoundArgs...> > : public true_type {};
+struct is_bind_expression<__bind<_Fp, _BoundArgs...> > : public true_type {};
 
 template<class _Rp, class _Fp, class ..._BoundArgs>
 class __bind_r
     : public __bind<_Fp, _BoundArgs...>
 {
     typedef __bind<_Fp, _BoundArgs...> base;
     typedef typename base::_Fd _Fd;
     typedef typename base::_Td _Td;
template <class ..._Args>
         operator()(_Args&& ...__args) const
         {
             typedef __invoke_void_return_wrapper<_Rp> _Invoker;
             return _Invoker::__call(static_cast<base const&>(*this), _VSTD::forward<_Args>(__args)...);
         }
 };
 
 template<class _Rp, class _Fp, class ..._BoundArgs>
-struct __is_bind_expression<__bind_r<_Rp, _Fp, _BoundArgs...> > : public true_type {};
+struct is_bind_expression<__bind_r<_Rp, _Fp, _BoundArgs...> > : public true_type {};
 
 template<class _Fp, class ..._BoundArgs>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
 __bind<_Fp, _BoundArgs...>
 bind(_Fp&& __f, _BoundArgs&&... __bound_args)
 {
     typedef __bind<_Fp, _BoundArgs...> type;
     return type(_VSTD::forward<_Fp>(__f), _VSTD::forward<_BoundArgs>(__bound_args)...);

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