[libcxx-commits] [PATCH] D124346: [libc++] Complete the implementation of N4190

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__config:1245
 #endif
 
 #if defined(_LIBCPP_HAS_NO_PRAGMA_PUSH_POP_MACRO)
----------------
For the types that are specified as inheriting from `unary_function`/`binary_function`, can you please add a test to check that we inherit from them in C++ <= 14?


================
Comment at: libcxx/include/__config:1235
+#if !defined(_LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION) && _LIBCPP_STD_VER > 14
+#  define _LIBCPP_ABI_NO_BINDER_BASES
+#endif
----------------
philnik wrote:
> ldionne wrote:
> > I don't understand why we're making an ABI change in the stable ABI?
> AFAICT this can only be ABI affecting if you have a class that is derived from both `unary_function` and another class that derives from it. Since there is no more `unary_function` to derive from, there is no way to have an ABI break. Same for `binary_function`.
I don't think that is correct. Indeed, let's consider:

```
template <class _Pred>
class binary_negate
    : public binary_function<_Pred::first_argument_type, _Pred::second_argument_type, bool>
{ ... };

template <class _Arg1, class _Arg2, class _Result>
class pointer_to_binary_function
    : public binary_function<_Arg1, _Arg2, _Result>
{ ... };
```

The exact classes don't matter, I just picked two classes that derived from `binary_function`. Now, let's take a look at the layout of the following type:

```
struct Foo
  : binary_negate<bool(int,int)>,
    pointer_to_binary_function<int, int, bool>
{ };
```

Without the bases, `sizeof(Foo)` is 1, and it is `2` with the bases. See https://godbolt.org/z/5b3PnjK54.

---------------------------------

This sucks because it makes this PR more complicated. One simple thing we could do is inherit from a new type called `__binary_function_or_empty <binary_negate<_Pred>, _Pred::first_argument_type, _Pred::second_argument_type, bool>` (and similarly for all function objects):

```
template <class> struct __empty { };
template <class, class, class> struct __binary_function_layout { };

#if _LIBCPP_STD_VER <= 14 || defined(_LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION)
template <class _Token, class, class, class>
using __binary_function_or_empty = binary_function<_Arg1, _Arg2, _Result>;
#elif defined(_LIBCPP_ABI_NO_BINDER_BASES)
template <class _Token, class, class, class>
using __binary_function_or_empty = __empty<_Token>;
#else
template <class _Token, class _Arg1, class _Arg2, class _Result>
using __binary_function_or_empty = __binary_function_layout<_Arg1, _Arg2, _Result>;
#endif
```

That way, we can define function objects unconditionally like

```
template <class _Pred>
class binary_negate
    : public __binary_function_or_empty<binary_negate<_Pred>, _Pred::first_argument_type, _Pred::second_argument_type, bool>
{ ... };
```

Since the `_Token` is guaranteed to be unique, we'll always get the EBO (🤞🏻).


================
Comment at: libcxx/include/__functional/binder2nd.h:24
 
 template <class __Operation>
 class _LIBCPP_TEMPLATE_VIS _LIBCPP_DEPRECATED_IN_CXX11 binder2nd
----------------
Unrelated, but we should rename this to `_Operation` (elsewhere too). Can you make a NFC commit?


================
Comment at: libcxx/include/ext/__hash:25
 template <> struct _LIBCPP_TEMPLATE_VIS hash<const char*>
+#ifndef _LIBCPP_ABI_NO_BINDER_BASES
  : public std::unary_function<const char*, size_t>
----------------
philnik wrote:
> ldionne wrote:
> > Technically, I think we still need to be providing the `argument_type` and `result_type` typedefs in C++17, but they should be deprecated. In practice, I don't know whether we care, it may be acceptable to just remove them. But whatever we do, we should be consistent between different specializations of `std::hash` (the current patch isn't, since e.g. `std::hash<std::bitset<...>>` provides those typedefs.
> > 
> > Furthermore, if we do provide them in C++17, they should be marked as deprecated.
> > 
> > In C++20, they should never be provided.
> I'm going with removing them, since they are removed in C++20 anyways.
I'm sorry for the churn, but I don't know why I said it was reasonable not to provide them. Looking at cppreference, they are clearly documented as being there in C++17, so I think we should provide them, but mark them as deprecated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124346/new/

https://reviews.llvm.org/D124346