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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/docs/ReleaseNotes.rst:96
+  you have to define ``_LIBCPP_DISABLE_DEPRECATION_WARNINGS``. Note that this disables
+  all deprectaiton warnings.
+
----------------
avogelsgesang wrote:
> deprectaiton -> deprecation
Still a typo!


================
Comment at: libcxx/include/__functional/binary_function.h:41
+
+template <class, class _Arg1, class _Arg2, class _Result> struct __binary_function_ebo_base {
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS)
----------------



================
Comment at: libcxx/include/__functional/hash.h:383
 
 _LIBCPP_SUPPRESS_DEPRECATED_PUSH
 template<class _Tp>
----------------
It seems to me like we can probably remove these `PUSH`/`POP` macros here now, since `unary_function` is used behind the `__unary_function_or_empty` alias, which has the right `PUSH`/`POP` macros when it is defined. Can you try removing them (here and elsewhere)?


================
Comment at: libcxx/include/__functional/mem_fn.h:43-45
+#if _LIBCPP_STD_VER > 14
+    typename invoke_result<type, _ArgTypes...>::type
+#else
----------------
Why is this needed?


================
Comment at: libcxx/include/__functional/unary_function.h:31
+
+template <class, class _Arg, class _Result> struct __unary_function_ebo_base {
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_BINDER_TYPEDEFS)
----------------



================
Comment at: libcxx/include/__functional/unary_negate.h:37-38
+
+    using argument_type = typename _Predicate::argument_type;
+    using result_type = bool;
 };
----------------
I don't think that should be there.


================
Comment at: libcxx/include/__functional/weak_result_type.h:232
 
 #ifndef _LIBCPP_CXX03_LANG
 // 3 or more arguments
----------------
Something for a different patch, but we do support variadics in C++03 so we can include this code block in all cases, unless I am mistaken.


================
Comment at: libcxx/include/string:4381-4382
 
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wdeprecated-declarations")
 template <class _CharT, class _Allocator>
----------------
I don't think you need to push/pop here?


================
Comment at: libcxx/include/string_view:911-912
 // [string.view.hash]
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wdeprecated-declarations")
 template<class _CharT>
----------------
Same.


================
Comment at: libcxx/include/system_error:436-437
 
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wdeprecated-declarations")
 template <>
----------------
Same.


================
Comment at: libcxx/include/thread:199-200
 
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wdeprecated-declarations")
 template<>
----------------
Same.


================
Comment at: libcxx/include/typeindex:104-105
 
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wdeprecated-declarations")
 template <>
----------------
Same.


================
Comment at: libcxx/include/vector:3227-3228
 
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wdeprecated-declarations")
 template <class _Allocator>
----------------
Same.


================
Comment at: libcxx/test/std/depr/depr.function.objects/depr.base/depr.verify.cpp:9-18
 // <functional>
-// REQUIRES: c++03 || c++11 || c++14
-// unary_function was removed in C++17
+// REQUIRES: c++11 || c++14
+// binary_function was removed in C++17
 
-// unary_function
+// check that binary_function is marked deprecated
 
 #include <functional>
----------------
Can you have one test for `binary_function` and one test for `unary_function`? Also, please make sure the comments at the top of the test are accurate.


================
Comment at: libcxx/test/std/diagnostics/syserr/syserr.hash/error_condition.pass.cpp:13
 // struct hash
-//     : public unary_function<T, size_t>
 // {
----------------
I think this can stay? And if it should go (because the Standard doesn't mandate that inheritance), then it should go from the other `hash` tests too.


================
Comment at: libcxx/test/std/diagnostics/syserr/syserr.hash/error_condition.pass.cpp:29
     typedef std::hash<T> H;
-    static_assert((std::is_same<H::argument_type, T>::value), "" );
-    static_assert((std::is_same<H::result_type, std::size_t>::value), "" );
----------------
I think this should be guarded with `#if TEST_STD_VER <= 14` instead like you've done elsewhere.


================
Comment at: libcxx/test/std/utilities/function.objects/refwrap/binder_typedefs.compile.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Thanks for adding this coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124346