This is an archive of the discontinued LLVM Phabricator instance.

[libc++][PSTL] Implement <execution> contents
ClosedPublic

Authored by philnik on Jan 16 2023, 7:53 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGebc05b93a11b: [libc++][PSTL] Implement <execution> contents

Diff Detail

Event Timeline

philnik created this revision.Jan 16 2023, 7:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 7:53 PM
philnik updated this revision to Diff 492449.Jan 26 2023, 7:49 AM

Try to fix CI

philnik updated this revision to Diff 492497.Jan 26 2023, 9:56 AM

Try to fix CI

philnik updated this revision to Diff 495184.Feb 6 2023, 9:45 AM

Try to fix CI

philnik updated this revision to Diff 500644.Feb 26 2023, 6:25 PM

Try to fix CI

philnik updated this revision to Diff 500646.Feb 26 2023, 6:31 PM

Update format ignorelist

philnik updated this revision to Diff 500762.Feb 27 2023, 6:24 AM

Try to fix CI

ldionne added inline comments.Mar 13 2023, 8:30 AM
libcxx/test/libcxx/utilities/expol/policies.compile.pass.cpp
9

Please add a synopsis for what you're testing here.

libcxx/test/std/utilities/expol/is_execution_policy.compile.pass.cpp
9

Same, synopsis. It allows us to know precisely what is being tested in a test. In this case it's not too hard to guess, but often there are multiple function overloads or different behaviors in different standard versions, so it's useful to keep that synopsis.

philnik updated this revision to Diff 516975.Apr 25 2023, 4:22 PM
philnik marked 2 inline comments as done.

Address comments

ldionne published this revision for review.Apr 26 2023, 9:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2023, 9:21 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Apr 26 2023, 9:43 AM

LGTM modulo comments.

libcxx/include/execution
1

You'll hate me but we need a synopsis!

59

Are those inline?

libcxx/include/pstl/internal/glue_execution_defs.h
1 ↗(On Diff #516975)

I think libcxx/include/pstl/internal/execution_defs.h should go as well, right?

Edit: It's too early for that, more cleanup is needed.

libcxx/test/libcxx/utilities/expol/policies.compile.pass.cpp
9

Let's add a comment explaining what you're testing here. It seems to be that it's QOI whether the policies can be copied or not, and you decided that they should only be passed by reference. I think it's fine to be strict since the standard allows us to be.

libcxx/test/std/utilities/expol/policies.compile.pass.cpp
15–18
32–39

Instead, let's make it a .pass.cpp test and do something like:

__noinline__ void use(auto&) { }

int main(int, char**) {
  auto& x = std::execution::unseq;
  use(std::execution::unseq);
  ASSERT_SAME_TYPE(x, ...);
  return 0;
}

This should link and run. Also, we should test this inside constexpr and outside constexpr. The benefit over what you're doing right now is that if we implemented these global objects e.g. externally in the dylib and forgot to define them, your current test would work just fine. Ideally this test would catch that problem.

This revision is now accepted and ready to land.Apr 26 2023, 9:43 AM
philnik updated this revision to Diff 517253.Apr 26 2023, 11:33 AM
philnik marked 5 inline comments as done.
  • Rebased
  • Address comments
This revision was landed with ongoing or failed builds.Apr 29 2023, 8:41 PM
This revision was automatically updated to reflect the committed changes.
libcxx/include/CMakeLists.txt