[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/docs/Status/Cxx17Papers.csv:1
 "Paper #","Group","Paper Name","Meeting","Status","First released version"
 "`N3911 <https://wg21.link/n3911>`__","LWG","TransformationTrait Alias ``void_t``\ .","Urbana","|Complete|","3.6"
----------------
We need to add a release note, since this will cause some code to start failing to compile (because of deprecation warnings and the removed classes).


================
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
----------------
I don't understand why we're making an ABI change in the stable ABI?


================
Comment at: libcxx/include/__functional/binary_function.h:24
 template <class _Arg1, class _Arg2, class _Result>
-struct _LIBCPP_TEMPLATE_VIS binary_function
+struct _LIBCPP_TEMPLATE_VIS _LIBCPP_DEPRECATED_IN_CXX11 binary_function
 {
----------------
Can you please add a test to check that this is deprecated in C++11? (same for others).


================
Comment at: libcxx/include/__functional/function.h:100
 };
+#endif // defined(_LIBCPP_ENABLE_CXX17_REMOVED_UNARY_BINARY_FUNCTION)
 
----------------
This comment is inconsistent with the `#if`'s condition. However, regardless, I think we should remove the `#if` entirely, since it is 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>
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124346