[libc++] mark barrier constructor as explicit in <barrier>

[diamante0018]

Summary

If I read the standard correctly, the public constructor of "barrier" should be marked as "constexpr explicit".
I see some of the internal classes used by the barrier header are correctly marked but I think, if I'm not mistaken, the standard would like the public class to have the correct definition as well.

Because the implementation that llvm uses by default is not constexpr friendly at this time, this revision will focus on only marking it as explicit.

[Mordante]

Thanks for working on this.

libcxx/include/barrier

297

Can you upload the new diff with context?

301

constexpr is implicitly inline, the same for functions inlined in the class.

301

Can you add a test this is really constexpr?

Please use our typical constexpr test method (for example test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp`.

int main(int, char**) {
  test();

#if TEST_STD_VER > 17
  static_assert(test());
#endif

  return 0;
}

301

For other reviewers I verified this wording was in the original paper https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1135r6.html

[diamante0018]

For clarification, that means that adding inline to all the functions in the class is redundant?
i.e. I can safely undo my changes and focus only on adding constexpr explicit to the ctor and adding a test.

[philnik]

There is no need to add inline everywhere. In these cases it most likely doesn't actually do anything. Please add tests that the constructor is constexpr and explicit. You can test the constexpr by adding a gloabl constinit variable and the explicit by making sure that is_convertible_v<...> is false.

[diamante0018]

libcxx/include/barrier

297

I should be able to address your review and upload the new additions here, yes. I hope that is what you mean. Thanks for your review

[diamante0018]

In D154590#4477808, @philnik wrote:

There is no need to add inline everywhere. In these cases it most likely doesn't actually do anything. Please add tests that the constructor is constexpr and explicit. You can test the constexpr by adding a gloabl constinit variable and the explicit by making sure that is_convertible_v<...> is false.

Understood, I will do that in an hour or so. Thanks for your review as well.

[Mordante]

libcxx/include/barrier

297

No with context means either using the arc tool or a diff -U999999. If you look at other patches, for example https://reviews.llvm.org/D153037 you can show all existing lines in the patch. That is useful for reviewers.

[diamante0018]

Address review: attempt to test for explicit & constexpr.

Please note: I was a bit confused by how explicit is tested here on LLVM. I most likely made a mistake, please let me know if I can improve the test file. Thanks

[diamante0018]

Fix format of my patch

[diamante0018]

Re-order header includes

[diamante0018]

Change function definition from bool to void

[diamante0018]

Update test file

[diamante0018]

Fix just the constexpr test

[diamante0018]

Update the test file again

[diamante0018]

I am unfortunately struggling a bit to come up with the proper syntax to test for 'excplicit' ctor.

I have tried a few things but I can't understand how test_convertible should be used. Could someone point me in the right direction?

What I tried but failed:

#if TEST_STD_VER >= 11
  using CompletionF = std::function<void()>;
  static_assert(!test_convertible<std::barrier, std::ptrdiff_t, CompletionF>(), "This constructor must be explicit");
#endif

and

static_assert(!test_convertible<std::barrier, std::ptrdiff_t, std::_CompletionF>(), "This constructor must be explicit"); // _CompletionF is not part of std

I am missing the point about how test_convertible should be used to test the ctor of a class that takes a function as a parameter

Thanks.

[diamante0018]

Update the test file

[Mordante]

Are you still having troubles with parts of this patch?

libcxx/test/std/thread/thread.barrier/ctor.pass.cpp

26

Can this header be included unconditionally?

31

This test does nothing.

[diamante0018]

In D154590#4485489, @Mordante wrote:

Are you still having troubles with parts of this patch?

Yes, unfortunately, despite me trying to understand already existing test files I was unable to figure out the proper syntax to test for explicit.
barrier's class takes two parameters, I don't know what syntax I should use for test_convertible because the second parameter is a function.
I might also need a bit of help for the constexpr test.
Thanks for asking.

[diamante0018]

libcxx/test/std/thread/thread.barrier/ctor.pass.cpp

26

Does not look like it. A lot of already existing tests file include this header conditionally. That's the general understanding I have of this file

[diamante0018]

Update test file for constexpr test

[diamante0018]

libcxx/test/std/thread/thread.barrier/ctor.pass.cpp

31

Does the updated version work now?

[diamante0018]

Use TEST_CONSTINIT to test if std::barrier is really constexpr

[diamante0018]

Fix test file for explicit

[diamante0018]

Remove unused header

[diamante0018]

Update test file according to review left on llvm's Discord server

[diamante0018]

Use TEST_CONSTEXPR_CXX20 macro

[diamante0018]

Update test file according to review left on llvm's Discord

[Mordante]

Thanks, LGTM. I leave the final approval to @philnik since he also had remarks.

[diamante0018]

Attempt to fix test failure

[diamante0018]

Fix static assert test

[diamante0018]

Use {} instead of () to initialize barrier object

[diamante0018]

Attempt to fix constexpr test

[diamante0018]

Make the internal classes constexpr as well so the static_assert may succeed

[diamante0018]

Fix format of my patch

[diamante0018]

The test is failing because __construct_barrier_algorithm_base is not a constexpr function.

[diamante0018]

I will abandon this revision, as a discussion happened on the llvm discord, the implementation is not constexpr friendly and will need some extra attention that is beyond my capabilities and the initial scope of this revision

[philnik]

In D154590#4490754, @diamante0018 wrote:

I will abandon this revision, as a discussion happened on the llvm discord, the implementation is not constexpr friendly and will need some extra attention that is beyond my capabilities and the initial scope of this revision

I think the explicit part should be uncontroversial, in case you want to continue with that.

[diamante0018]

In D154590#4490760, @philnik wrote:

In D154590#4490754, @diamante0018 wrote:

I will abandon this revision, as a discussion happened on the llvm discord, the implementation is not constexpr friendly and will need some extra attention that is beyond my capabilities and the initial scope of this revision

I think the explicit part should be uncontroversial, in case you want to continue with that.

Okay, I will continue that if you wish. Allow me push a diff without the constexpr changes.

[diamante0018]

Only apply "explicit"

[philnik]

LGTM with green CI and comment addressed. If you don't have commit access, please provide your name and email for attribution.

libcxx/test/std/thread/thread.barrier/ctor.pass.cpp

27–31

You can make this a .compile.pass.cpp and remove the main.

[diamante0018]

Address review

[diamante0018]

Sure, I addressed your review so it should be even better now (no main function in the test file). I don't have write access so my details are the following:

Edoardo Sanguineti <[e-mail]>

[diamante0018]

Still some CI issues on the MAC runners, I think it's caused by an extra compiler flag that is leftover. I will remove the flag and try again

libcxx/include/barrier

 template<class _CompletionF = __empty_completion>
 class barrier {
 
     __barrier_base<_CompletionF> __b_;
 public:
     using arrival_token = typename __barrier_base<_CompletionF>::arrival_token;
 
     static _LIBCPP_HIDE_FROM_ABI constexpr ptrdiff_t max() noexcept {
         return __barrier_base<_CompletionF>::max();
     }
 
     _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
-    barrier(ptrdiff_t __count, _CompletionF __completion = _CompletionF())
+    constexpr explicit barrier(ptrdiff_t __count, _CompletionF __completion = _CompletionF())
         : __b_(__count, _VSTD::move(__completion)) {
     }
 
     barrier(barrier const&) = delete;
     barrier& operator=(barrier const&) = delete;
 
     [[__nodiscard__]] _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY
     arrival_token arrive(ptrdiff_t __update = 1)

libcxx/test/std/thread/thread.barrier/ctor.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: no-threads
+// UNSUPPORTED: c++03, c++11
+
+// XFAIL: availability-synchronization_library-missing
+
+// <barrier>
+
+// constexpr explicit barrier(ptrdiff_t __count, _CompletionF __completion = _CompletionF());
+
+// Make sure that the ctor of barrier is constexpr and explicit.
+
+#include <barrier>
+#include <cassert>
+
+#include "test_macros.h"
+#if TEST_STD_VER >= 11
+#include "test_convertible.h"
+#endif
+
+template <class B>
+TEST_CONSTEXPR_CXX20 bool test() {
+  B(5);
+  return true;
+}
+
+int main(int, char**) {
+  assert(test<std::barrier<>>());
+
+#if TEST_STD_VER > 17
+  static_assert(test<std::barrier<>>());
+#endif
+
+#if TEST_STD_VER >= 11
+  static_assert(!test_convertible<std::barrier, std::ptrdiff_t>(), "This constructor must be explicit");
+#endif
+
+  return 0;
+}