[libcxx-commits] [PATCH] D55840: P0722R3: Implement library support for destroying delete

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: include/new:169
+
+struct destroying_delete_t {
+  explicit destroying_delete_t() = default;
----------------
jwakely wrote:
> jwakely wrote:
> > jwakely wrote:
> > > ldionne wrote:
> > > > EricWF wrote:
> > > > > mclow.lists wrote:
> > > > > > ckennelly wrote:
> > > > > > > ldionne wrote:
> > > > > > > > Please guard with > C++17. We should not enable features in older standards unless we have a compelling reason to.
> > > > > > > If there's compiler support (we're checking _impl), would it be reasonable to provide it with C++17 as well?
> > > > > > 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.
> > > > > > 
> > > > > 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.
> > > > I agree, this should be `#if _LIBCPP_STD_VER > 17 || defined(__cpp_impl_destroying_delete)`.
> > > 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.
> > > 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".
> 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.
> 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.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55840