P0722R3: Implement library support for destroying delete

[EricWF]

Summary

This provides the std::destroying_delete_t declaration in C++2a and after. (Even when the compiler doesn't support the language feature).

However, the feature test macro __cpp_lib_destroying_delete is only defined when we have both language support and C++2a.

[mclow.lists]

This is going to need some work before landing.

• The feature test macro should be defined in <version>, not in <new>
• Testing for the feature-test macro should go into the feature test stuff; look in test/std/language.support/support.limits/support.limits.general.

• Should we have a libc++ macro _LIBCPP_HAS_NO_DESTROYING_DELETE, which gets defined in <config>, and then used in <new>? [ I am unsure; if there was a new replaceable overload of operator delete then I would say "absolutely". ]

[mclow.lists]

Another question - why do we need to check if clang supports destroying delete to define the type destroying_delete_t? Why not define it always, and just define the feature test macro when we have compiler support?

[mclow.lists]

You should also update www/cxx2a_status.html to show that P0722 is "complete".

[ldionne]

In D55840#1348304, @mclow.lists wrote:

Another question - why do we need to check if clang supports destroying delete to define the type destroying_delete_t? Why not define it always, and just define the feature test macro when we have compiler support?

I'm not a big fan of this approach. It seems better to define __cpp_lib_destroying_delete if and only if we provide destroying_delete_t, which makes sense if and only if the compiler has support for it (i.e. __cpp_impl_destroying_delete is defined).

include/new

37

Please note that those are C++20 additions.

169

Please guard with > C++17. We should not enable features in older standards unless we have a compelling reason to.

[ckennelly]

In D55840#1348416, @ldionne wrote:

In D55840#1348304, @mclow.lists wrote:

Another question - why do we need to check if clang supports destroying delete to define the type destroying_delete_t? Why not define it always, and just define the feature test macro when we have compiler support?

I'm not a big fan of this approach. It seems better to define __cpp_lib_destroying_delete if and only if we provide destroying_delete_t, which makes sense if and only if the compiler has support for it (i.e. __cpp_impl_destroying_delete is defined).

P1353 describes the impl macro as "whether the library feature can be provided," so that implied guarding exposing the library features with that macro as well.

include/new

169

If there's compiler support (we're checking _impl), would it be reasonable to provide it with C++17 as well?

[mclow.lists]

include/new

169

Not reasonable, no.
I did this one, for something that I thought was a slam-dunk (string_view in pre-c++17) and it has been nothing but a series of small annoyances.

[ldionne]

Requesting changes so it shows up in my review queue properly.

[EricWF]

include/new

167

Please use the generate_feature_test_macro_components.py script to handle defining and testing the feature test macros.

169

This is a separate question from std::string_view. This is a core-language feature that needs library support. The compiler decides whether this feature is on or off.

Like aligned allocation and sized delete, the library should provide destroying_delete_t whenever the compiler defines its language feature test macro. Otherwise, Clang can't do its job. This approach allows users to opt-into important new language features as an extension, or to opt-out of them when availability is a concern (this is particularly important for allocation/deallocation functions).

We should enable the typedef when either _LIBCPP_STD_VER > 17 or defined(__cpp_impl_destroying_delete). The latter macro signify that Clang needs us to provide the type because *clang* has chosen to provide this language feature as an extension.

[EricWF]

Question:

In C++20 we should declare destroying_delete_t, even if __cpp_impl_destroying_delete isn't defined.
But in that case should be define __cpp_lib_destroying_delete? The library has the type, but users
can't actually perform destroying delete.

Since __cpp_impl_destroyng_delete isn't for users, but __cpp_lib_destroying_delete is, then I don't think we should define it unless the compiler actually provides the language feature.

[ldionne]

In D55840#1382153, @EricWF wrote:

Question:

In C++20 we should declare destroying_delete_t, even if __cpp_impl_destroying_delete isn't defined.
But in that case should be define __cpp_lib_destroying_delete? The library has the type, but users
can't actually perform destroying delete.

Since __cpp_impl_destroyng_delete isn't for users, but __cpp_lib_destroying_delete is, then I don't think we should define it unless the compiler actually provides the language feature.

I think I agree. We should only define __cpp_lib_destroying_delete when the feature can be used sanely. TBH, I don't think that's a question the committee even thought about when introducing feature-test macros? There has to be an expected behaviour for us stdlib implementers here.

[ldionne]

include/new

169

I agree, this should be #if _LIBCPP_STD_VER > 17 || defined(__cpp_impl_destroying_delete).

[jwakely]

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89345#c3 for what I've done in libstdc++

tl;dr the type and variable depend on C++2a, the __cpp_lib macro depends on the type being defined and the compiler defining the impl macro to say it will recognise the type.

include/new

169

That will break for C++98 mode, because constexpr (and defaulted special members and inline variables) gives an error in C++98. That's the problem I was asked to fix in libstdc++, to be compatible with Clang.

[jwakely]

In D55840#1395019, @ldionne wrote:

TBH, I don't think that's a question the committee even thought about when introducing feature-test macros?

Well when we introduced feature test macros we didn't have any of these "the compiler defines the impl part and the library defines the one for end users" cases. I think the intention is that the library macro tells users if the feature actually works, and so it should definitely depend on the compiler promising to keep its side of the bargain, which is what the impl macro tells you.

[jwakely]

include/new

169

This approach allows users to opt-into important new language features as an extension, or to opt-out of them when availability is a concern (this is particularly important for allocation/deallocation functions).

If there was a -fdestroying-delete option to opt in or out of the compiler feature, I'd agree. But IIUC correctly, clang enables it unconditionally, so there's no opt-out, and no way for users to say "please don't define destroying_delete in C++11 mode, I use that name as a macro because I'm annoying like that".

[jwakely]

include/new

169

Also at https://reviews.llvm.org/D55741#1360693 Richard says Clang expects the library to decide whether to provide it, and you're saying "Clang has decided to provide it, so we should respect that" ... you can't both defer to the other, you should decide who's in charge here :-)
My decision for libstdc++ was to not make it available pre-C++2a, for the sake of strict conformance and simplicity.

[ldionne]

include/new

169

If there was a -fdestroying-delete option to opt in or out of the compiler feature, I'd agree.

This. I revert to my original stance of "we should only provide it in C++20 and above", unless there's a way that users can actually opt-in with some Clang flag.

[EricWF]

Hijacking the revision with Chris's blessing.

include/new

169

:-(

I was wrong. Clang enables destroying delete unconditionally, and somewhat carelessly.

We need to find another mechanism allowing users to opt in.

[EricWF]

This re-works how we allow using destroying delete as an extension.

First, it turns the extension off by default. Even if Clang would support it.
Second, it adds the _LIBCPP_ENABLE_DESTROYING_DELETE_EXT which can be used to enable the declarations of std::destroying_delete_t and std::destroying_delete in any dialect.

Finally, __cpp_lib_destroying_delete is defined only when libc++ provides the library declarations and the compiler supports the language features.

[ldionne]

Please explain to me why we should have this in pre-C++20 dialects.

include/new

37

This comment stands, those declarations should have // C++20 in the synopsis.

169

See, that's one reason why I don't like back-porting recent features to older dialects.

[EricWF]

In D55840#1434761, @ldionne wrote:

Please explain to me why we should have this in pre-C++20 dialects.

My users get meaningful performance improvements with this feature, but we can't upgrade everything to C++20 just yet.
For portable code, having different performance characteristics between C++17 and C++20 is a problem.

Destroying delete is a language feature first; much like aligned allocation and sized deallocation.
Given a cooperative standard library, Clang offers these features as extensions. We should cooperate.

Like aligned allocation, destroying delete is off by default and requires user opt-in.

Allowing this extension provides value to users. And it is consistent with our handling of
aligned allocation and sized deallocation.

include/new

169

I don't love it either. But it's not as bad as it seams. The explicit constructor prevents {} from converting to destroying_delete_t. But in C++03 {} isn't a thing, so we don't have to worry about it [1].

Clang offers the language extension in C++03. It just needs a destroying_delete_t definition. Users want to use destroying delete before C++20. We allow similar extensions for aligned allocation and sized deallocation.

[1] https://godbolt.org/z/EV2Efy

[EricWF]

• Address inline review comments.
• Update the feature test macro script.

[ldionne]

In D55840#1435158, @EricWF wrote:

In D55840#1434761, @ldionne wrote:

Please explain to me why we should have this in pre-C++20 dialects.

My users get meaningful performance improvements with this feature, but we can't upgrade everything to C++20 just yet.

This is true for almost any C++20 feature. This isn't a reason to backport everything that we can to earlier standards. I don't want to litter the library with opt-ins like _LIBCPP_ENABLE_DESTROYING_DELETE_EXT, as it adds complexity and it creates more flavours of the library that we should be testing.

We're just turning in circles here since we've had that discussion before and we just disagree more fundamentally on providing extensions. Would you be willing not to tie this review to providing an extension so we can have a proper policy discussion to solve this matter once and for all?

test/std/language.support/support.dynamic/destroying_delete_t.pass.cpp

6

This is the old license.

[EricWF]

Implement the minimal feature without extension as requested.

[EricWF]

Accepting after addressing Louis comments.

[zoecarver]

I don't think P0722 got checked off in the 2a status.

docs/FeatureTestMacroTable.rst

.. table:: Current Status
     ``__cpp_lib_char8_t``                             ``201811L``      
     ------------------------------------------------- -----------------
     ``__cpp_lib_concepts``                            *unimplemented*  
     ------------------------------------------------- -----------------
     ``__cpp_lib_constexpr_misc``                      *unimplemented*  
     ------------------------------------------------- -----------------
     ``__cpp_lib_constexpr_swap_algorithms``           *unimplemented*  
     ------------------------------------------------- -----------------
-    ``__cpp_lib_destroying_delete``                   *unimplemented*  
+    ``__cpp_lib_destroying_delete``                   ``201806L``      
     ------------------------------------------------- -----------------
     ``__cpp_lib_erase_if``                            ``201811L``      
     ------------------------------------------------- -----------------
     ``__cpp_lib_generic_unordered_lookup``            *unimplemented*  
     ------------------------------------------------- -----------------
     ``__cpp_lib_interpolate``                         ``201902L``      
     ------------------------------------------------- -----------------
     ``__cpp_lib_is_constant_evaluated``               ``201811L``      

include/new

 
 class bad_array_new_length : public bad_alloc // C++14
 {
 public:
     bad_array_new_length() noexcept;
 };
 
 enum class align_val_t : size_t {}; // C++17
+
+struct destroying_delete_t { // C++20
+  explicit destroying_delete_t() = default;
+};
+inline constexpr destroying_delete_t destroying_delete{}; // C++20
+
 struct nothrow_t {};
 extern const nothrow_t nothrow;
 typedef void (*new_handler)();
 new_handler set_new_handler(new_handler new_p) noexcept;
 new_handler get_new_handler() noexcept;
 
 // 21.6.4, pointer optimization barrier
 template <class T> constexpr T* launder(T* p) noexcept; // C++17
#if !defined(_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION) && \
     !defined(_LIBCPP_ABI_VCRUNTIME)
 #ifndef _LIBCPP_CXX03_LANG
 enum class _LIBCPP_ENUM_VIS align_val_t : size_t { };
 #else
 enum align_val_t { __zero = 0, __max = (size_t)-1 };
 #endif
 #endif
 
+#if _LIBCPP_STD_VER > 17
+// Enable the declaration even if the compiler doesn't support the language
+// feature.
+struct destroying_delete_t {
+  explicit destroying_delete_t() = default;
+};
+_LIBCPP_INLINE_VAR constexpr destroying_delete_t destroying_delete{};
+#endif // _LIBCPP_STD_VER > 17
+
 }  // std
 
 #if defined(_LIBCPP_CXX03_LANG)
 #define _THROW_BAD_ALLOC throw(std::bad_alloc)
 #else
 #define _THROW_BAD_ALLOC
 #endif
 

include/version

 // # define __cpp_lib_bind_front                           201811L
 // # define __cpp_lib_bit_cast                             201806L
 # if !defined(_LIBCPP_NO_HAS_CHAR8_T)
 #   define __cpp_lib_char8_t                            201811L
 # endif
 // # define __cpp_lib_concepts                             201806L
 // # define __cpp_lib_constexpr_misc                       201811L
 // # define __cpp_lib_constexpr_swap_algorithms            201806L
-// # define __cpp_lib_destroying_delete                    201806L
+# if _LIBCPP_STD_VER > 17 && defined(__cpp_impl_destroying_delete) && __cpp_impl_destroying_delete >= 201806L
+#   define __cpp_lib_destroying_delete                  201806L
+# endif
 # define __cpp_lib_erase_if                             201811L
 // # define __cpp_lib_generic_unordered_lookup             201811L
 # define __cpp_lib_interpolate                          201902L
 # if !defined(_LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED)
 #   define __cpp_lib_is_constant_evaluated              201811L
 # endif
 // # define __cpp_lib_list_remove_return_type              201806L
 // # define __cpp_lib_ranges                               201811L
 // # define __cpp_lib_three_way_comparison                 201711L
 #endif
 
 #endif // _LIBCPP_VERSIONH

test/std/language.support/support.dynamic/destroying_delete_t.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// struct destroying_delete_t {
+//   explicit destroying_delete_t() = default;
+// };
+// inline constexpr destroying_delete_t destroying_delete{};
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
+
+#include <new>
+
+#include <cassert>
+#include "test_macros.h"
+
+struct A {
+  void *data;
+  A();
+  ~A();
+
+  static A* New();
+  void operator delete(A*, std::destroying_delete_t);
+};
+
+bool A_constructed = false;
+bool A_destroyed = false;
+bool A_destroying_deleted = false;
+
+A::A() {
+  A_constructed = true;
+}
+
+A::~A() {
+  A_destroyed = true;
+}
+
+A* A::New() {
+  return new(::operator new(sizeof(A))) A();
+}
+
+void A::operator delete(A* a, std::destroying_delete_t) {
+  A_destroying_deleted = true;
+  ::operator delete(a);
+}
+
+#ifndef __cpp_lib_destroying_delete
+#error "Expected __cpp_lib_destroying_delete to be defined"
+#elif __cpp_lib_destroying_delete < 201806L
+#error "Unexpected value of __cpp_lib_destroying_delete"
+#endif
+
+int main() {
+  // Ensure that we call the destroying delete and not the destructor.
+  A* ap = A::New();
+  assert(A_constructed);
+  delete ap;
+  assert(!A_destroyed);
+  assert(A_destroying_deleted);
+}

test/std/language.support/support.limits/support.limits.general/new.version.pass.cpp

 #   error "__cpp_lib_launder should be defined in c++17"
 # endif
 # if __cpp_lib_launder != 201606L
 #   error "__cpp_lib_launder should have the value 201606L in c++17"
 # endif
 
 #elif TEST_STD_VER > 17
 
-# if !defined(_LIBCPP_VERSION)
+# if TEST_STD_VER > 17 && defined(__cpp_impl_destroying_delete) && __cpp_impl_destroying_delete >= 201806L
 #   ifndef __cpp_lib_destroying_delete
 #     error "__cpp_lib_destroying_delete should be defined in c++2a"
 #   endif
 #   if __cpp_lib_destroying_delete != 201806L
 #     error "__cpp_lib_destroying_delete should have the value 201806L in c++2a"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_destroying_delete
-#     error "__cpp_lib_destroying_delete should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_destroying_delete should not be defined when TEST_STD_VER > 17 && defined(__cpp_impl_destroying_delete) && __cpp_impl_destroying_delete >= 201806L is not defined!"
 #   endif
 # endif
 
 # ifndef __cpp_lib_hardware_interference_size
 #   error "__cpp_lib_hardware_interference_size should be defined in c++2a"
 # endif
 # if __cpp_lib_hardware_interference_size != 201703L
 #   error "__cpp_lib_hardware_interference_size should have the value 201703L in c++2a"

test/std/language.support/support.limits/support.limits.general/version.version.pass.cpp

 #     error "__cpp_lib_constexpr_swap_algorithms should have the value 201806L in c++2a"
 #   endif
 # else // _LIBCPP_VERSION
 #   ifdef __cpp_lib_constexpr_swap_algorithms
 #     error "__cpp_lib_constexpr_swap_algorithms should not be defined because it is unimplemented in libc++!"
 #   endif
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if TEST_STD_VER > 17 && defined(__cpp_impl_destroying_delete) && __cpp_impl_destroying_delete >= 201806L
 #   ifndef __cpp_lib_destroying_delete
 #     error "__cpp_lib_destroying_delete should be defined in c++2a"
 #   endif
 #   if __cpp_lib_destroying_delete != 201806L
 #     error "__cpp_lib_destroying_delete should have the value 201806L in c++2a"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_destroying_delete
-#     error "__cpp_lib_destroying_delete should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_destroying_delete should not be defined when TEST_STD_VER > 17 && defined(__cpp_impl_destroying_delete) && __cpp_impl_destroying_delete >= 201806L is not defined!"
 #   endif
 # endif
 
 # ifndef __cpp_lib_enable_shared_from_this
 #   error "__cpp_lib_enable_shared_from_this should be defined in c++2a"
 # endif
 # if __cpp_lib_enable_shared_from_this != 201603L
 #   error "__cpp_lib_enable_shared_from_this should have the value 201603L in c++2a"

utils/generate_feature_test_macro_components.py

feature_test_macros = sorted([ add_version_header(x) for x in [
    "headers": ["string", "deque", "forward_list", "list", "vector", "map",
                "set", "unordered_map", "unordered_set"]
   },
   {"name": "__cpp_lib_destroying_delete",
    "values": {
      "c++2a": 201806L,
    },
    "headers": ["new"],
-   "unimplemented": True,
+   "depends":
+      "TEST_STD_VER > 17"
+      " && defined(__cpp_impl_destroying_delete)"
+      " && __cpp_impl_destroying_delete >= 201806L",
+   "internal_depends":
+      "_LIBCPP_STD_VER > 17"
+      " && defined(__cpp_impl_destroying_delete)"
+      " && __cpp_impl_destroying_delete >= 201806L",
    },
   {"name": "__cpp_lib_three_way_comparison",
    "values": {
      "c++2a": 201711L,
    },
    "headers": ["compare"],
    "unimplemented": True,
    },