[libcxx-commits] [PATCH] D110042: [libc++] counting_semaphore should not be default-constructible

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

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp:25-26
+
+static_assert(!std::is_default_constructible_v<std::binary_semaphore>, "");
+static_assert(!std::is_default_constructible_v<std::counting_semaphore<>>, "");
+
----------------
ldionne wrote:
> These would belong in a `ctor.default.pass.cpp` file, and we should also run the default ctor.
I think you missed the `!`. ;)  These static-asserts may be out of place here, I admit, but I have no idea where else to put them.


================
Comment at: libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp:32-33
+#if 0 // TODO FIXME: the ctor should be constexpr when TEST_STD_VER > 17
+constinit std::binary_semaphore bs(1);
+constinit std::counting_semaphore cs(1);
+#endif
----------------
ldionne wrote:
> Please follow the usual pattern of running the code in a conditionally-`constexpr` function and calling it from `main`.
Unfortunately we cannot do that, because `counting_semaphore` is constexpr-constructible but //not// constexpr-destructible. (I almost asked "what is the use of such a constexpr constructor" in Slack myself, but as I was typing the question, figured out that the purpose is so that we can use semaphores with C++20 `constinit`. So that's what I did here.)


================
Comment at: libcxx/test/std/thread/thread.semaphore/max.pass.cpp:24
+  static_assert(std::counting_semaphore<std::numeric_limits<int>::max()>::max() >= std::numeric_limits<int>::max(), "");
+  static_assert(std::counting_semaphore<std::numeric_limits<ptrdiff_t>::max()>::max() == std::numeric_limits<ptrdiff_t>::max(), "");
   return 0;
----------------
@ldionne wrote:
> I would be curious to see what the patch looks like for using <atomic> unconditionally under the hood. It may be reasonable to make that ABI break if we can make it ASAP - we haven't officially released it anywhere I think, but LLVM 13 would be the first release so we have to be quick (as in within a few days if it's not already too late).

>From lurking the email list, I surmise that Tom has held up the release like three times now, and doesn't want to hold it up more, so I'd guess it counts as "too late." But I could make the PR this afternoon, I guess, just to see what it looks like.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110042