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

Sat Apr 30 07:23:05 PDT 2022

philnik marked 5 inline comments as done.
philnik added inline comments.

================
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
----------------
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`.

================
Comment at: libcxx/include/__functional/function.h:100
 };
+#endif // defined(_LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION)

----------------
ldionne wrote:
> This comment is inconsistent with the `#if`'s condition. However, regardless, I think we should remove the `#if` entirely, since it is always valid.
How is the condition always valid?

================
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>
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124346