[libcxx-commits] [PATCH] D70117: [libc++][P0174] Deprecated/removed parts of default allocator.

[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/memory:118
 public:
-    typedef size_t                                size_type;
-    typedef ptrdiff_t                             difference_type;
-    typedef T*                                    pointer;
-    typedef const T*                              const_pointer;
-    typedef typename add_lvalue_reference<T>::type       reference;
-    typedef typename add_lvalue_reference<const T>::type const_reference;
-    typedef T                                     value_type;
+    typedef size_t    size_type;                         // deprecated in C++17, removed in C++20
+    typedef ptrdiff_t difference_type;                   // deprecated in C++17, removed in C++20
----------------
Woooh, thanks for the cleanup.


================
Comment at: libcxx/include/memory:1361
 __has_allocate_hint_test(_Alloc&& __a, _SizeType&& __sz, _ConstVoidPtr&& __p)
+    _LIBCPP_SUPPRESS_DEPRECATED_PUSH
     -> decltype((void)__a.allocate(__sz, __p), true_type());
----------------
What! I didn't know you could surround literally any piece of code with these pragmas! It does make sense though, since it's a preprocessor thing. Clang will really take the pragma into account when parsing just that part of the declaration?


================
Comment at: libcxx/include/memory:1394
+    _LIBCPP_SUPPRESS_DEPRECATED_PUSH
+    -> decltype(__a.construct(__p, _VSTD::forward<_Args>(__args)...), true_type());
+    _LIBCPP_SUPPRESS_DEPRECATED_POP
----------------
If the return type of `__a.construct(...)` somehow hijacks `operator,`, this won't work. But I don't care, don't change it.

An Allocator's `construct` method is supposed to return `void` -- if someone's that clever, they deserve to be taught better.


================
Comment at: libcxx/include/memory:1871
         {
-        if (__n > max_size())
+        // TODO(mpark): Replace with `allocator_traits<allocator>::max_size(*this)`.
+        if (__n > (size_t(~0) / sizeof(_Tp)))
----------------
I'm OK with this TODO since I saw you fixed it in a subsequent patch.


================
Comment at: libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.depr_in_cxx17.fail.cpp:15-16
 
-#include <memory>
-#include <cassert>
 
----------------
Wait, you shouldn't get rid of this test pre-C++17. You should mark it as `REQUIRES: c++03, ..., c++14`. Or did I miss something and you did not actually remove this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70117