[PATCH] D108697: [Coroutines] [libcxx] Move coroutine components out from std::experimental namespace
Arthur O'Dwyer via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 7 15:39:15 PDT 2021
Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.
First pass. This is still presumably a step in the right direction, but it's currently far from being "C++20 conforming" and IMHO it would probably be nice to get it conforming (or close to conforming) before it lands.
================
Comment at: libcxx/include/coroutine:47-48
+#include <type_traits>
+#include <functional>
+#include <memory> // for hash<T*>
+#include <cstddef>
----------------
Now that we're doing granular headers, please prefer to `#include` just the smallest helper headers possible (e.g. in this case I think all you need is `<__functional/hash.h>` for `hash<T*>`).
================
Comment at: libcxx/include/coroutine:58-60
+_LIBCPP_WARNING("<experimental/coroutine> cannot be used with this compiler")
+#else
+#warning <experimental/coroutine> cannot be used with this compiler
----------------
Remove `experimental/`
================
Comment at: libcxx/include/coroutine:86
+ _LIBCPP_INLINE_VISIBILITY
+ _LIBCPP_CONSTEXPR coroutine_handle() _NOEXCEPT : __handle_(nullptr) {}
+
----------------
(1) I'd prefer to see `__handle_` get a default member initializer on its declaration, so we don't have to member-initialize it here nor on line 89.
(2) If this codepath were limited to `_LIBCPP_STD_VER > 11`, then we could use `constexpr` throughout instead of `_LIBCPP_CONSTEXPR`, and `noexcept` instead of `_NOEXCEPT`. Can we do that? Should we?
================
Comment at: libcxx/include/coroutine:107
+ _LIBCPP_INLINE_VISIBILITY
+ void resume() {
+ _LIBCPP_ASSERT(__is_suspended(), "resume() can only be called on suspended coroutines");
----------------
`void resume() const`.
Also `void operator()() const` on line 104.
(yes, this smells wrong, but it's how it was standardized, according to https://eel.is/c++draft/coroutine.handle#general )
================
Comment at: libcxx/include/coroutine:108
+ void resume() {
+ _LIBCPP_ASSERT(__is_suspended(), "resume() can only be called on suspended coroutines");
+ _LIBCPP_ASSERT(!done(), "resume() has undefined behavior when the coroutine is done");
----------------
Grammar nit: `resume() can be called only on suspended coroutines`
================
Comment at: libcxx/include/coroutine:114
+ _LIBCPP_INLINE_VISIBILITY
+ void destroy() {
+ _LIBCPP_ASSERT(__is_suspended(), "destroy() can only be called on suspended coroutines");
----------------
`void destroy() const`
================
Comment at: libcxx/include/coroutine:127
+ _LIBCPP_INLINE_VISIBILITY
+ static coroutine_handle from_address(void* __addr) _NOEXCEPT {
+ coroutine_handle __tmp;
----------------
This should be marked `constexpr`.
================
Comment at: libcxx/include/coroutine:133-135
+ // FIXME: Should from_address(nullptr) be allowed?
+ _LIBCPP_INLINE_VISIBILITY
+ static coroutine_handle from_address(nullptr_t) _NOEXCEPT { return coroutine_handle(nullptr); }
----------------
Yes, it must be allowed, at least according to https://eel.is/c++draft/coroutine.handle.export.import ; but you shouldn't have this overload. Lines 126-131 above are already doing the right thing for both null and non-null pointers.
================
Comment at: libcxx/include/coroutine:137-141
+ template <class _Tp, bool _CallIsValid = false>
+ static coroutine_handle from_address(_Tp*) {
+ static_assert(_CallIsValid, "coroutine_handle<void>::from_address cannot be called with "
+ "non-void pointers");
+ }
----------------
This overload looks wrong; remove it. And add a test; something like this:
```
std::coroutine_handle<> h1 = [...];
int *p = (int*)h1.to_address();
std::coroutine_handle<> h2 = std::coroutine_handle<>::from_address(p);
```
================
Comment at: libcxx/include/coroutine:154
+
+// 18.11.2.7 comparison operators:
+inline _LIBCPP_INLINE_VISIBILITY bool operator==(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
----------------
Use stablenames throughout: e.g., `// [coroutine.handle.compare]`
================
Comment at: libcxx/include/coroutine:155-160
+inline _LIBCPP_INLINE_VISIBILITY bool operator==(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
+ return __x.address() == __y.address();
+}
+inline _LIBCPP_INLINE_VISIBILITY bool operator!=(coroutine_handle<> __x, coroutine_handle<> __y) _NOEXCEPT {
+ return !(__x == __y);
+}
----------------
Question for experts: Are we allowed to make this operator a hidden friend? If we're allowed to, then I strongly think we should make it a hidden friend.
In C++20 `operator!=` is not needed. (And the C++20 standard doesn't provide it.)
Ditto, C++20 specifies that `operator<=>` exists (and I'd like it to be a hidden friend), not the four `< <= > >=` operators.
================
Comment at: libcxx/include/coroutine:179-185
+#ifndef _LIBCPP_CXX03_LANG
+ // 18.11.2.1 construct/reset
+ using coroutine_handle<>::coroutine_handle;
+#else
+ _LIBCPP_INLINE_VISIBILITY coroutine_handle() _NOEXCEPT : _Base() {}
+ _LIBCPP_INLINE_VISIBILITY coroutine_handle(nullptr_t) _NOEXCEPT : _Base(nullptr) {}
+#endif
----------------
Here we could just do
```
_LIBCPP_HIDE_FROM_ABI coroutine_handle() noexcept = default;
_LIBCPP_HIDE_FROM_ABI coroutine_handle(nullptr_t) noexcept {}
```
because default-constructing a `_Base` already has the right behavior. Right?
But actually, whoa, C++20 definitely does //not// permit us to make an inheritance relationship between `coroutine_handle<>` and `coroutine_handle<Promise>`! These need to be two unrelated class types.
https://eel.is/c++draft/coroutine.handle#general
================
Comment at: libcxx/include/coroutine:205-222
+ // NOTE: this overload isn't required by the standard but is needed so
+ // the deleted _Promise* overload doesn't make from_address(nullptr)
+ // ambiguous.
+ // FIXME: should from_address work with nullptr?
+ _LIBCPP_INLINE_VISIBILITY
+ static coroutine_handle from_address(nullptr_t) _NOEXCEPT { return coroutine_handle(nullptr); }
+
----------------
Remove all of this.
================
Comment at: libcxx/include/coroutine:238
+template <>
+class _LIBCPP_TEMPLATE_VIS coroutine_handle<noop_coroutine_promise> : public coroutine_handle<> {
+ using _Base = coroutine_handle<>;
----------------
This inheritance relationship shouldn't be here.
https://eel.is/c++draft/support.coroutine#coroutine.noop
================
Comment at: libcxx/include/coroutine:285-290
+template <class _Tp>
+struct hash<_VSTD::coroutine_handle<_Tp> > {
+ using __arg_type = _VSTD::coroutine_handle<_Tp>;
+ _LIBCPP_INLINE_VISIBILITY
+ size_t operator()(__arg_type const& __v) const _NOEXCEPT { return hash<void*>()(__v.address()); }
+};
----------------
Nits:
```
template <class _Tp>
struct hash<coroutine_handle<_Tp> > {
_LIBCPP_HIDE_FROM_ABI
size_t operator()(const coroutine_handle<_Tp>& __v) const noexcept { return hash<void*>()(__v.address()); }
};
```
================
Comment at: libcxx/include/coroutine:296
+
+#endif /* _LIBCPP_COROUTINE */
----------------
Nit: `//` not `/*` (compare to some other header for consistency)
================
Comment at: libcxx/test/libcxx/language.support/support.coroutines/version.pass.cpp:20
-int main(int, char**)
-{
-
- return 0;
-}
+int main(int, char**) { return 0; }
----------------
This file is autogenerated; you shouldn't change its whitespace.
(In general, you should ignore clang-format's wibbling and just follow existing style.)
================
Comment at: libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.compare/less_comp.pass.cpp:55
+int main(int, char**) {
+ std::pair<uintptr_t, uintptr_t> const TestCases[] = {{0, 0}, {16, 16}, {0, 16}, {16, 0}};
for (auto& TC : TestCases) {
----------------
Undo this whitespace change.
================
Comment at: libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.export/from_address.fail.cpp:30-33
+ // expected-error at coroutine:* 3 {{coroutine_handle<void>::from_address cannot be called with non-void pointers}}
+ H::from_address((int*)nullptr); // expected-note {{requested here}}
H::from_address((const void*)nullptr); // expected-note {{requested here}}
H::from_address((const char*)nullptr); // expected-note {{requested here}}
----------------
This test will have to change, since all of these are conforming C++20 and shouldn't be erroring.
================
Comment at: libcxx/test/std/language.support/support.coroutines/coroutine.handle/coroutine.handle.hash/hash.pass.cpp:51-52
for (auto& TC : TestCases) {
- do_test<coro::coroutine_handle<>>(TC.first, TC.second);
- do_test<coro::coroutine_handle<int>>(TC.first, TC.second);
+ do_test<std::coroutine_handle<> >(TC.first, TC.second);
+ do_test<std::coroutine_handle<int> >(TC.first, TC.second);
}
----------------
You added some extra spaces here; remove them. (Ditto in several other test files.)
================
Comment at: libcxx/utils/generate_header_tests.py:53
- "experimental/coroutine": ["if defined(__cpp_coroutines)"],
+ "coroutine": ["if defined(__cpp_coroutines)"],
"experimental/regex": ["ifndef _LIBCPP_HAS_NO_LOCALIZATION"],
----------------
a-z plz
================
Comment at: llvm/utils/gn/secondary/libcxx/include/BUILD.gn:362
"clocale",
+ "coroutine",
"cmath",
----------------
a-z plz; but also, what is this file? I've never seen it before and we (libc++) definitely haven't been maintaining it. For example, it's missing headers like "concepts" and "compare" and "ranges". But it has "semaphore", so it's not just C++17 stuff... What //is// this?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108697/new/
https://reviews.llvm.org/D108697
More information about the llvm-commits
mailing list