[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:
> > 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.)




================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique.sizezero.pass.cpp:11
+
+#include <memory>
+#include <cassert>
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Should there be a similar test for `std::shared_ptr`?
> `shared_ptr` doesn't have any similar completeness check, does it?
No. I just wondered about it since it's the other smart pointer type.


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

https://reviews.llvm.org/D120633