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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 2 inline comments as done.
Quuxplusone 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;
----------------
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. :)


================
Comment at: libcxx/test/std/ranges/range.access/begin.sizezero.pass.cpp:23
+};
+static_assert(sizeof(A) == 0); // an extension supported by GCC and Clang
+
----------------
Mordante wrote:
> I think it would be better to move this comment to line 11 to describe what the test is about.
> 
> Let's make sure the test also works on MSVC, where the size is 4. That will probably make @CaseyCarter happy.
Sure, I can say something like
```
// std::ranges::begin
// std::ranges::cbegin
//   Test an element type that is complete, yet has size zero.
```
I definitely //don't// want this test to silently compile when `sizeof(A) > 0`, because then someone might accidentally change `m`'s declaration so that `sizeof(A) > 0` on GCC and Clang, and then it would be silently nerfed. I don't object to adding `XFAIL: msvc`, though. And if @CaseyCarter knows a way to achieve `sizeof(A) == 0` on MSVC, that'd be even better!


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique.sizezero.pass.cpp:11
+
+#include <memory>
+#include <cassert>
----------------
Mordante wrote:
> Should there be a similar test for `std::shared_ptr`?
`shared_ptr` doesn't have any similar completeness check, does it?


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

https://reviews.llvm.org/D120633