[libc++] counting_semaphore should not be default-constructible

[• Quuxplusone]

Summary

Neither the current C++2b draft, nor any revision of [p1135],
nor libstdc++, claims that `counting_semaphore` should be
default-constructible. I think this was just a copy-paste issue
somehow.

Also, `explicit` was missing from the constructor.

Also, `constexpr` was missing; but that's probably more of a
technical limitation, since apparently there are some platforms
where we don't (can't??) use the atomic implementation and
have to rely on pthreads, which obviously isn't constexpr.
Let's see what buildkite thinks of the `constexpr` keyword here.

Indeed, buildkite doesn't like the constexpr ( https://buildkite.com/llvm-project/libcxx-ci/builds/5432 ), so I'll remove it for now, but I think we should consider taking a quick ABI break to get us onto the atomic-based implementation everywhere. Otherwise I don't really see what the path forward is, as C++ keeps cranking out standards; we can't get stuck with a pre-C++20 semaphore forever, right?

[• Quuxplusone]

Remove constexpr from the "bad" semaphore_base implementation; leave it on counting_semaphore's public API, for the benefit of platforms already using the atomic_base implementation.

[ldionne]

CI is failing because you can't use the _v aliases before C++14, please use ::value instead.

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

libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp

26–27

These would belong in a ctor.default.pass.cpp file, and we should also run the default ctor.

33–34

Please follow the usual pattern of running the code in a conditionally-constexpr function and calling it from main.

[• Quuxplusone]

libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp

26–27

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.

33–34

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

libcxx/test/std/thread/thread.semaphore/max.pass.cpp

24

@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.

[• Quuxplusone]

Use ::value not _v.

[ldionne]

LGTM with my suggestion, which should fix the CI.

libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp

13–15

You need to remove this since this is a compile.pass.cpp test -- it won't fail because of back-deployment.

33–34

Understood, thanks for explaining.

[• Quuxplusone]

Remove XFAIL from compile-only test.

libcxx/include/semaphore

 */
 
 class __atomic_semaphore_base
 {
     __atomic_base<ptrdiff_t> __a;
 
 public:
     _LIBCPP_INLINE_VISIBILITY
-    __atomic_semaphore_base(ptrdiff_t __count) : __a(__count)
+    constexpr explicit __atomic_semaphore_base(ptrdiff_t __count) : __a(__count)
     {
     }
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void release(ptrdiff_t __update = 1)
     {
         if(0 < __a.fetch_add(__update, memory_order_release))
             ;
         else if(__update > 1)

 */
 
 class __platform_semaphore_base
 {
     __libcpp_semaphore_t __semaphore;
 
 public:
     _LIBCPP_INLINE_VISIBILITY
-    __platform_semaphore_base(ptrdiff_t __count) :
+    explicit __platform_semaphore_base(ptrdiff_t __count) :
         __semaphore()
     {
         __libcpp_semaphore_init(&__semaphore, __count);
     }
     _LIBCPP_INLINE_VISIBILITY
     ~__platform_semaphore_base() {
         __libcpp_semaphore_destroy(&__semaphore);
     }
class counting_semaphore
     __semaphore_base<__least_max_value> __semaphore;
 
 public:
     static constexpr ptrdiff_t max() noexcept {
         return __least_max_value;
     }
 
     _LIBCPP_INLINE_VISIBILITY
-    counting_semaphore(ptrdiff_t __count = 0) : __semaphore(__count) { }
+    constexpr explicit counting_semaphore(ptrdiff_t __count) : __semaphore(__count) { }
     ~counting_semaphore() = default;
 
     counting_semaphore(const counting_semaphore&) = delete;
     counting_semaphore& operator=(const counting_semaphore&) = delete;
 
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     void release(ptrdiff_t __update = 1)
     {

libcxx/test/std/thread/thread.semaphore/binary.pass.cpp

 // macOS 11.0.
 // XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
 
 // <semaphore>
 
 #include <semaphore>
 #include <chrono>
 #include <thread>
+#include <type_traits>
 
 #include "make_test_thread.h"
 #include "test_macros.h"
 
+static_assert(std::is_same<std::binary_semaphore, std::counting_semaphore<1>>::value, "");
+
 int main(int, char**)
 {
   std::binary_semaphore s(1);
 
   auto l = [&](){
     for(int i = 0; i < 1024; ++i) {
         s.acquire();
         std::this_thread::sleep_for(std::chrono::microseconds(1));

libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+// UNSUPPORTED: c++03, c++11
+
+// <semaphore>
+
+// constexpr explicit counting_semaphore(ptrdiff_t desired);
+
+#include <semaphore>
+#include <type_traits>
+
+#include "test_macros.h"
+
+static_assert(!std::is_default_constructible<std::binary_semaphore>::value, "");
+static_assert(!std::is_default_constructible<std::counting_semaphore<>>::value, "");
+
+static_assert(!std::is_convertible<int, std::binary_semaphore>::value, "");
+static_assert(!std::is_convertible<int, std::counting_semaphore<>>::value, "");
+
+#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

libcxx/test/std/thread/thread.semaphore/max.pass.cpp

 
 #include <semaphore>
 #include <thread>
 
 #include "test_macros.h"
 
 int main(int, char**)
 {
-  static_assert(std::counting_semaphore<>::max() > 0, "");
+  static_assert(std::counting_semaphore<>::max() >= 1, "");
   static_assert(std::counting_semaphore<1>::max() >= 1, "");
-  static_assert(std::counting_semaphore<std::numeric_limits<int>::max()>::max() >= 1, "");
-  static_assert(std::counting_semaphore<std::numeric_limits<ptrdiff_t>::max()>::max() >= 1, "");
-  static_assert(std::counting_semaphore<1>::max() == std::binary_semaphore::max(), "");
+  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;
 }