This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Quuxplusone on Sep 19 2021, 12:30 PM.

Details

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?

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Sep 19 2021, 12:30 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSep 19 2021, 12:30 PM
Quuxplusone edited the summary of this revision. (Show Details)Sep 19 2021, 1:53 PM

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.

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.

ldionne requested changes to this revision.Sep 20 2021, 1:54 PM
This revision now requires changes to proceed.Sep 20 2021, 1:54 PM
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.

Use ::value not _v.

ldionne accepted this revision.Sep 20 2021, 5:02 PM

LGTM with my suggestion, which should fix the CI.

libcxx/test/std/thread/thread.semaphore/ctor.compile.pass.cpp
12–14

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.

This revision is now accepted and ready to land.Sep 20 2021, 5:02 PM

Remove XFAIL from compile-only test.

This revision was landed with ongoing or failed builds.Sep 21 2021, 1:20 PM
This revision was automatically updated to reflect the committed changes.