[PATCH] D53120: Implement sized deallocation for std::allocator and friends.

[Louis Dionne via Phabricator]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

> C++14 sized deallocation is disabled by default due to ABI concerns.

@EricWF Can you expand on those ABI concerns? Do you mean because support for those is not provided in the dylib shipped by Apple?

> On Apple this patch makes no attempt to determine if the sized operator delete is unavailable, only that the language feature is enabled.

You mean there's no check like there is for aligned allocation? For example (in `__config`):

  #if defined(__APPLE__)
  #  if defined(__MAC_OS_X_VERSION_MIN_REQUIRED)
  #    if __MAC_OS_X_VERSION_MIN_REQUIRED < 1060
  #      define _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
  #    endif
  #  endif
  #endif // defined(__APPLE__)

I think there should be one -- users won't expect calls to `std::allocator::deallocate` to suddenly start failing after this patch when they target an OS version that does not have sized deallocation. I think Apple started shipping support for sized deallocation in 10.12, so the check should be:

  #if defined(__APPLE__)
  #  if defined(__MAC_OS_X_VERSION_MIN_REQUIRED)
  #    if __MAC_OS_X_VERSION_MIN_REQUIRED < 101200
  #      define _LIBCPP_HAS_NO_LIBRARY_SIZED_DEALLOCATION
  #    endif
  #  endif
  #endif // defined(__APPLE__)



================
Comment at: include/new:290
+ private:
+  static inline void __do_deallocate_handle_size(void *__ptr, size_t __size) {
+#ifdef _LIBCPP_HAS_NO_SIZED_DEALLOCATION
----------------
I think you need `_LIBCPP_INLINE_VISIBILITY` here and on other private functions too.


================
Comment at: src/experimental/memory_resource.cpp:37
+    virtual void do_deallocate(void * __p, size_t __n, size_t __align)
+        { _VSTD::__libcpp_deallocate(__p, __n, __align); /* FIXME */ }
 
----------------
Do you know why there's a FIXME here? (I know it was there before your patch, I'm just curious)


================
Comment at: test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp:10
+
+// test libc++'s implementation of align_val_t, and the relevent new/delete
+// overloads in all dialects when -faligned-allocation is present.
----------------
typo: relevant


https://reviews.llvm.org/D53120