This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by diamante0018 on Jul 6 2023, 3:05 AM.

Details

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.

Diff Detail

Event Timeline

diamante0018 created this revision.Jul 6 2023, 3:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2023, 3:05 AM
diamante0018 requested review of this revision.Jul 6 2023, 3:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2023, 3:05 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
diamante0018 retitled this revision from [libc++] mark barrier constructor as constexpr explicit to [libc++] mark barrier constructor as constexpr explicit in <barrier>.Jul 6 2023, 10:34 AM

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

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 requested changes to this revision.Jul 6 2023, 10:40 AM

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.

This revision now requires changes to proceed.Jul 6 2023, 10:40 AM
diamante0018 added inline comments.Jul 6 2023, 10:40 AM
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

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 added inline comments.Jul 6 2023, 10:44 AM
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 edited the summary of this revision. (Show Details)

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

Fix format of my patch

diamante0018 marked 3 inline comments as done.Jul 6 2023, 12:37 PM

Re-order header includes

Change function definition from bool to void

Update test file

Fix just the constexpr test

Update the test file again

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.

Update the test file

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.

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 added inline comments.Jul 10 2023, 8:55 AM
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

Update test file for constexpr test

diamante0018 added inline comments.Jul 10 2023, 9:02 AM
libcxx/test/std/thread/thread.barrier/ctor.pass.cpp
31

Does the updated version work now?

diamante0018 retitled this revision from [libc++] mark barrier constructor as constexpr explicit in <barrier> to [libc++] mark barrier constructor as constexpr and explicit in <barrier>.

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

Fix test file for explicit

Remove unused header

diamante0018 marked an inline comment as done.Jul 10 2023, 9:49 AM

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

Use TEST_CONSTEXPR_CXX20 macro

diamante0018 marked 2 inline comments as done.Jul 10 2023, 10:18 AM

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

Mordante accepted this revision as: Mordante.Jul 10 2023, 10:35 AM

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

Attempt to fix test failure

Fix static assert test

Use {} instead of () to initialize barrier object

Attempt to fix constexpr test

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

Fix format of my patch

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

diamante0018 abandoned this revision.Jul 11 2023, 12:02 PM

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 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 reclaimed this revision.EditedJul 11 2023, 12:04 PM

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 retitled this revision from [libc++] mark barrier constructor as constexpr and explicit in <barrier> to [libc++] mark barrier constructor as explicit in <barrier>.
diamante0018 edited the summary of this revision. (Show Details)

Only apply "explicit"

philnik accepted this revision.Jul 11 2023, 12:17 PM

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.

This revision is now accepted and ready to land.Jul 11 2023, 12:17 PM

Address review

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 <edoardo.sanguineti222@gmail.com>

diamante0018 marked an inline comment as done.Jul 11 2023, 12:27 PM

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

This revision was landed with ongoing or failed builds.Jul 12 2023, 12:31 PM
This revision was automatically updated to reflect the committed changes.