lewissbaker marked 40 inline comments as done. lewissbaker added a comment.
Thanks very much for the detailed review @CaseyCarter! Very much appreciated :) ================ Comment at: include/experimental/__memory:80 +{ + return (__s + __a - 1) & ~(__a - 1); +} ---------------- CaseyCarter wrote: > This is missing preconditions that `__a` is a power of 2, and that `__s <= > -__a`. Is there a recommended way of documenting/implementing these preconditions on a constexpr function in libc++? The previous version that lived in `<experimental/memory_resource>` was not marked constexpr and so was able to use `_LIBCPP_ASSERT`. ================ Comment at: include/experimental/task:28 +#if defined(_LIBCPP_WARNING) +_LIBCPP_WARNING("<experimental/task> cannot be used with this compiler") +#else ---------------- CaseyCarter wrote: > "requires a compiler with support for coroutines" would be more informative. This messaging was copied from <experimental/coroutine>. I'll update the message there as well. ================ Comment at: include/experimental/task:141 + + void* __pointer = __charAllocator.allocate( + __get_padded_frame_size_with_allocator<_CharAlloc>(__size)); ---------------- CaseyCarter wrote: > The return value of `allocate` isn't necessarily convertible to `void*`, it > could be a fancy pointer. We should either `static_assert(is_same_v<typename > allocator_traits<_CharAlloc>::void_pointer, void*>, "Piss off with your fancy > pointers");` or use `pointer_traits` here and in `__deallocFunc` to unfancy > and re-fancy the pointer. I'm not quite sure how to go about using `pointer_traits` here when interacting with coroutines and allocators. Under C++20 I guess I would do something like: ``` typename allocator_traits<_CharAlloc>::void_pointer __pointer = __charAllocator.allocate(...); ... return _VSTD::to_address(__pointer); ``` I assume that `operator new()` still needs to return `void*` rather than the fancy pointer so we need the `to_address` call in there. Is there some fallback for `to_address()` for unfancying the pointer in earlier standard versions? Maybe we should just go with the `static_assert()` for now? ================ Comment at: include/experimental/task:220 +public: + __task_promise() _NOEXCEPT : __state_(_State::__no_value) {} + ---------------- CaseyCarter wrote: > This mem-initializer for `__state_` is redundant with the default member > initializer on 306. I'll remove the default member initializer on 306 then. We can't `= default` the constructor here due to the union member containing types with non-trivial constructors/destructors. ================ Comment at: include/experimental/task:244 + exception_ptr(current_exception()); + __state_ = _State::__exception; +#else ---------------- CaseyCarter wrote: > Are you certain that `unhandled_exception` can't possibly be called after > storing a value? If so, this would leak the value. Yes, I think there is a case where this could happen. If we execute a `co_return someValue` which calls `promise.return_value()` and constructs `__value_`. Then while executing the `goto final_suspend;` if any of the destructors of in-scope variables throw an exception which then escapes the coroutine body we could end up calling `unhandled_exception()` with `__value_` already having been constructed. Similarly, the exception thrown from a destructor could be caught and then a new `co_return someOtherValue;` executed. So we should probably be guarding against `__value_` containing a value inside `return_value()` as well. ================ Comment at: include/experimental/task:308 + union { + char __empty_; + _Tp __value_; ---------------- CaseyCarter wrote: > These `__empty_` members seem extraneous. The `__empty_` members were added at your request ;) See https://reviews.llvm.org/D46140?id=144168#inline-403822 I can remove them if you think they're not necessary. ================ Comment at: include/experimental/task:309 + char __empty_; + _Tp __value_; + exception_ptr __exception_; ---------------- CaseyCarter wrote: > Should we `static_assert` that `_Tp` is a destructible object type? Sure, will do. I wonder if we should make this a requirement in the wording of [P1056](https://wg21.link/P1056)? What do you think @GorNishanov? ================ Comment at: include/experimental/task:373 +template <> +class __task_promise<void> final : public __task_promise_base { + using _Handle = coroutine_handle<__task_promise>; ---------------- CaseyCarter wrote: > Do we care about `task<cv-void>`? I don't feel strongly either way. It's not something I've ever had a use for, but maybe it should be done for completeness? ================ Comment at: include/experimental/task:389 + + void __rvalue_result() { __throw_if_exception(); } + ---------------- CaseyCarter wrote: > Should these `__foovalue_result` members be `foo`-qualified to make them > harder to misuse? I'm not sure if it makes it much safer. We already have 'rvalue' in the method name which gives a hint to its move-ness. It's currently only called in one place: ``` return this->__coro_.promise().__rvalue_result(); ``` would become: ``` return std::move(this->__coro_.promise()).__rvalue_result(); ``` ================ Comment at: include/experimental/task:407 + using promise_type = __task_promise<_Tp>; + +private: ---------------- CaseyCarter wrote: > Similar to above, should we `static_assert` that `_Tp` is `void`, an lvalue > reference type, or a destructible non-array object type? Sure, can do. Further, to support `task<T>::operator co_await() &&` T needs to both be destructible and move-constructible. For `task<T>::operator co_await() &` T only needs to be destructible. ================ Comment at: include/experimental/task:453 + decltype(auto) await_resume() { + return this->__coro_.promise().__lvalue_result(); + } ---------------- CaseyCarter wrote: > `this->` is extraneous in these classes that derive from the concrete > `_AwaiterBase`. (And on 470) I get compile-errors if I remove the `this->`. ``` libcxx/include/experimental/task:502:16: error: 'std::experimental::coroutines_v1::task<counted>::__coro_' is not a member of class '_Awaiter' return __coro_.promise().__rvalue_result(); ``` Compiler bug? ================ Comment at: test/std/experimental/task/awaitable_traits.hpp:17 + +_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_COROUTINES + ---------------- CaseyCarter wrote: > It's super sketchy for test code to inject things into the production > namespace and to use reserved names. It's also inappropriate to use libc++ > internals in a test in the `std` tree. (Occurs repeatedly.) These were intended to be eventually added to the <experimental/coroutine> header and under the std::experimental namespace which is why they were using these macros. See [P1288R0](https://wg21.link/P1288R0). Similarly for `sync_wait()` which is proposed in [P1171R0](https://wg21.link/P1171R0). Is there some common namespace that test utilities should be declared within? If not I'll just move them to global scope and de-uglify them for now. ================ Comment at: test/std/experimental/task/counted.hpp:90 +#define DEFINE_COUNTED_VARIABLES() \ + std::size_t counted::nextId_; \ + std::size_t counted::defaultConstructedCount_; \ ---------------- CaseyCarter wrote: > Should `nextId_` be initialized to `1`, as it is in `reset` on 40? The intent was that any test would always initialise these values by calling reset() before running. I hadn't intended the static-initialization to "do the right thing" here but I can if you think that's preferable. ================ Comment at: test/std/experimental/task/manual_reset_event.hpp:47 + + __event_->__awaitingCoroutine_ = __coro; + ---------------- CaseyCarter wrote: > Why are we concerned about data races on `__event_->__state_`, but not > `__event_->__awaitingCoroutine_`? Only the awaiting thread writes to `__awaitingCoroutine_` and publishes this value when it writes to `__state_` by setting the state to `__not_set_waiting_coroutine`. A thread that calls `.set()` will only read from `__awaitingCoroutine_` if, when it updates the `__state_` variable, sees that the previous state was `__not_set_waiting_coroutine`. So the write the `__awaitingCoroutine_` should strictly happens-before any read from `__awaitingCoroutine_`. ================ Comment at: test/std/experimental/task/sync_wait.hpp:36-37 + __isSet_ = true; + } + __cv_.notify_all(); + } ---------------- CaseyCarter wrote: > lewissbaker wrote: > > The call to `notify_all()` needs to be inside the lock. > > Otherwise, it's possible the waiting thread may see the write to `__isSet_` > > inside `wait()` below and return, destroying the `condition_variable` > > before `__cv_.notify_all()` call completes. > The write to `__isSet_` must be under the lock, the call to `notify_all` need > not be. (Not that I care either way, just clarifying.) The problem here is that once we write `true` to `__isSet_` and release the lock that the thread calling `wait()` can potentially return and the on to destroy the `__oneshot_event` object, leaving the current thread with a dangling reference to a destroyed `std::condition_variable`. I ran into a similar problem in cppcoro. See https://github.com/lewissbaker/cppcoro/issues/98#issuecomment-450695759 ================ Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:18 +#include <memory> +#include <experimental/memory_resource> + ---------------- CaseyCarter wrote: > This is not a Standard or Coroutines TS header. I can't `#include <memory_resource>` as libc++ doesn't currently provide that header. How should I go about writing a test that makes sure that std::experimental::task works with `std::experimental::pmr::polymorphic_allocator` (or `std::pmr::polymorphic_allocator` if available)? ================ Comment at: test/std/experimental/task/task.basic/task_custom_allocator.pass.cpp:54 + my_allocator(my_allocator&& other) + : totalAllocated_(std::move(other.totalAllocated_)) + { ---------------- CaseyCarter wrote: > This is almost certainly going to cause problems, since it leaves the > moved-from allocator invalid. (And on 68.) Isn't a moved-from allocator left in a valid but unspecified state? ie. you can't assume it's ok to use. but should be ok to destruct or assign to. Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46140/new/ https://reviews.llvm.org/D46140 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits