[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

lewissbaker added a comment. So am I correct in understanding that the main issue with the chicken/egg problem for updating both the compiler to use the new stdlib facilities and updating the stdlib facilities is that we don't want to issue warnings about using `` and telling users to use ``

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

lewissbaker added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4450 +Value *Extra = Builder.CreateSelect(Cmp, Diff, Zero); +return RValue::get(Builder.CreateAdd(SizeCall, Extra)); } rjmccall wrote: > lewissbaker wrote: > > ychen wrote:

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

lewissbaker added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4450 +Value *Extra = Builder.CreateSelect(Cmp, Diff, Zero); +return RValue::get(Builder.CreateAdd(SizeCall, Extra)); } ychen wrote: > rjmccall wrote: > > ychen wrote: > > >

[PATCH] D82314: [RFC][Coroutines] Optimize the lifespan of temporary co_await object

lewissbaker added a comment. > There are still room for improvements, in particular, GCC has a 4K frame > for this function. I think GCC having a smaller coroutine frame is probably because it does not yet implement the allocation-elision optimisation which inlines the nested coroutine

[PATCH] D82415: [Coroutines] Special handle __builtin_coro_resume for final_suspend nothrow check

lewissbaker accepted this revision. lewissbaker added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/Sema/SemaCoroutine.cpp:621 +// returns a handle. In that case, even __builtin_coro_resume is not +// declared as

[PATCH] D82029: [Coroutines] Ensure co_await promise.final_suspend() does not throw

lewissbaker added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10521 +def err_coroutine_promise_final_suspend_requires_nothrow : Error< + "all code generated by co_await __promise.final_suspend() must not throw" +>; I'd perhaps

[PATCH] D46140: [coroutines] Add std::experimental::task type

lewissbaker marked an inline comment as done. lewissbaker added a comment. Gentle ping. Is there anything else people would like to see changed? Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46140/new/ https://reviews.llvm.org/D46140

[PATCH] D46140: [coroutines] Add std::experimental::task type

lewissbaker marked 3 inline comments as done. lewissbaker added inline comments. Comment at: include/experimental/task:160 +__get_allocator<_CharAlloc>(__pointer, __size); +_CharAlloc __allocatorOnStack = _VSTD::move(__allocatorInFrame); +

[PATCH] D46140: [coroutines] Add std::experimental::task type

lewissbaker updated this revision to Diff 192944. lewissbaker marked 6 inline comments as done. lewissbaker added a comment. This updated diff should address most of @CaseyCarter's review comments. For detailed changelog you can find individual changes here

[PATCH] D46140: [coroutines] Add std::experimental::task type

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

[PATCH] D46140: [coroutines] Add std::experimental::task type

lewissbaker marked an inline comment as done. lewissbaker added a comment. @EricWF This implementation will currently not work with MSVC as MSVC does not yet support the symmetric-transfer capability added to the coroutines specification in P0913R0 . Is MSVC a target

[PATCH] D46140: [coroutines] Add std::experimental::task type

lewissbaker updated this revision to Diff 192384. lewissbaker added a comment. Added missing 'require coroutines' for experimental/task entry in module.modulemap Added missing #include in experimental/task header that was failing module builds Repository: rCXX libc++ CHANGES SINCE LAST

[PATCH] D46140: [coroutines] Add std::experimental::task type

lewissbaker reopened this revision. lewissbaker added a comment. This revision is now accepted and ready to land. Reopening as the commit for this diff was reverted due to it breaking the buildbot module builds. Repository: rCXX libc++ CHANGES SINCE LAST ACTION

[PATCH] D46140: [coroutines] std::task type (WIP)

lewissbaker updated this revision to Diff 192208. lewissbaker added a comment. Fix race condition in __oneshot_event::set() method used by sync_wait(). Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D46140/new/ https://reviews.llvm.org/D46140 Files:

[PATCH] D46140: [coroutines] std::task type (WIP)

lewissbaker updated this revision to Diff 192203. lewissbaker edited the summary of this revision. lewissbaker added a comment. Herald added subscribers: libcxx-commits, jdoerfert. - Don't use __allocator as variable name (it is #defined to NASTY_MACRO). - Remove workaround in test for

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

lewissbaker added inline comments. Comment at: lib/Sema/SemaCoroutine.cpp:675 + // Second emphasis of [expr.await]p2: must be outside of an exception handler. + if (S.getCurScope()->getFlags() & Scope::CatchScope) { +S.Diag(Loc, diag::err_coroutine_within_handler) <<

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

lewissbaker added inline comments. Comment at: test/SemaCXX/coroutines.cpp:299-311 // FIXME: The spec says this is ill-formed. void operator=(CtorDtor&) { co_yield 0; // expected-error {{'co_yield' cannot be used in a copy assignment operator}} } void

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

lewissbaker added inline comments. Comment at: lib/Sema/SemaCoroutine.cpp:197-200 - if (S.isUnevaluatedContext()) { -S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword; -return false; - } Does removing this check now mean that we're not

[PATCH] D46140: [coroutines] std::task type (WIP)

lewissbaker added inline comments. Comment at: test/std/experimental/task/sync_wait.hpp:36-37 + __isSet_ = true; +} +__cv_.notify_all(); + } The call to `notify_all()` needs to be inside the lock. Otherwise, it's possible the waiting thread may see

[PATCH] D46140: [coroutines] std::task type (WIP)

lewissbaker updated this revision to Diff 156641. lewissbaker edited the summary of this revision. lewissbaker added a comment. - Make `task` constructor from `coroutine_handle` private. - Run clang-format over . https://reviews.llvm.org/D46140 Files: include/CMakeLists.txt

[PATCH] D46140: [coroutines] std::task type (WIP)

lewissbaker updated this revision to Diff 156565. lewissbaker added a comment. Herald added subscribers: cfe-commits, ldionne, mgorny. Many improvements: - Remove move-assignment operator - `task::operator co_await()` now returns by value when awaiting `task` rvalue. - Explicitly scope calls to

[PATCH] D45121: [coroutines] Add noop_coroutine to

lewissbaker added inline comments. Comment at: include/experimental/coroutine:294 + +inline _LIBCPP_ALWAYS_INLINE +noop_coroutine_handle noop_coroutine() _NOEXCEPT { EricWF wrote: > This should just be `_LIBCPP_INLINE_VISIBILITY`. We try not to use >

[PATCH] D45121: [coroutines] Add noop_coroutine to

lewissbaker accepted this revision. lewissbaker added a comment. This revision is now accepted and ready to land. The `coroutine_handle` type does not have a `from_address()` or a `from_promise()` static functions in the same way that the `coroutine_handle` implementation does. Is this