[libcxx-commits] [PATCH] D141888: [libc++][PSTL] Implement <execution> contents

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM modulo comments.



================
Comment at: libcxx/include/execution:1
 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
----------------
You'll hate me but we need a synopsis!


================
Comment at: libcxx/include/execution:37
+
+constexpr sequenced_policy seq{__disable_user_instantiations_tag{}};
+
----------------
Are those `inline`?


================
Comment at: libcxx/include/pstl/internal/glue_execution_defs.h:1
-// -*- C++ -*-
-//===----------------------------------------------------------------------===//
----------------
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.


================
Comment at: libcxx/test/libcxx/utilities/expol/policies.compile.pass.cpp:8
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14
----------------
ldionne wrote:
> Please add a synopsis for what you're testing here.
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.


================
Comment at: libcxx/test/std/utilities/expol/policies.compile.pass.cpp:14-17
+// inline constexpr sequenced_policy = implementation-defined;
+// inline constexpr parallel_policy = implementation-defined;
+// inline constexpr parallel_unsequenced_policy = implementation-defined;
+// inline constexpr unsequenced_policy = implementation-defined; // since C++20
----------------



================
Comment at: libcxx/test/std/utilities/expol/policies.compile.pass.cpp:31-38
+static_assert(std::is_same_v<remove_cvref_t<decltype(std::execution::seq)>, std::execution::sequenced_policy>);
+static_assert(std::is_same_v<remove_cvref_t<decltype(std::execution::par)>, std::execution::parallel_policy>);
+static_assert(
+    std::is_same_v<remove_cvref_t<decltype(std::execution::par_unseq)>, std::execution::parallel_unsequenced_policy>);
+
+#if TEST_STD_VER >= 20
+static_assert(std::is_same_v<remove_cvref_t<decltype(std::execution::unseq)>, std::execution::unsequenced_policy>);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141888/new/

https://reviews.llvm.org/D141888