[libcxx-commits] [PATCH] D120633: [libc++] Better handling for zero-sized types

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__memory/unique_ptr.h:50
   _LIBCPP_INLINE_VISIBILITY void operator()(_Tp* __ptr) const _NOEXCEPT {
-    static_assert(sizeof(_Tp) > 0,
-                  "default_delete can not delete incomplete type");
-    static_assert(!is_void<_Tp>::value,
-                  "default_delete can not delete incomplete type");
+    static_assert(sizeof(_Tp) >= 0, "cannot delete an incomplete type");
     delete __ptr;
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > Mordante wrote:
> > > > I would like a comment explaining why `sizeof(_Tp) == 0` is possible.
> > > > 
> > > > Looking at the number of `sizeof(_Tp) >= 0` in the code, would it make sense to add a type trait `__is_complete` and add the explanation there?
> > > > I would like a comment explaining why `sizeof(_Tp) == 0` is possible.
> > > 
> > > I'm not sure what such a comment would say. Would it just point to the regression test, like
> > > ```
> > > // This is correct. See test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique.sizezero.pass.cpp
> > > ```
> > > I mean, removing or changing the condition will end up pointing to that test anyway. For me, the important thing here is //consistency//: we want every instance of a completeness check across the whole codebase to use the same phrasing (`sizeof(_Tp) >= 0`), so that as long as we get it right once we get it right for every case.
> > > 
> > > Re `concept __is_complete`, see https://quuxplusone.github.io/blog/2021/12/27/libstdcxx-what-are-you-doing/ and D108645 and probably a few other passing mentions in different reviews. :)
> > The problem I have with the test is that types with size 0 don't exist according to C++. So it warrants an explanation why we need to test for 0.
> > 
> > I'm not sure why you don't want a `__is_complete` concept, but use the `incomplete` in the wording of the assert. So it seems we need to be able to test for completeness in libc++ at some places. (I get the point of the blog post that we should be careful about whether we need to test for completeness everywhere.)
> > 
> > 
> > I'm not sure why you don't want a `__is_complete` concept [...] it seems we need to be able to test for completeness in libc++ at some places
> 
> We want to test for completeness //right here//, but we must scrupulously avoid caching/memoizing that computed result anywhere that might become stale later on in the TU (including in a concept, type trait, or variable template).
> Godbolt link copied from D108645: https://godbolt.org/z/nr3dPY7va
Ah thanks, I forgot about the memoizing. I still think it would be good to add a comment why sizeof == 0 is intended.


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

https://reviews.llvm.org/D120633