Conversation
|
@andreasbuhr Something isn't right here, the PR says that you extracted parts of my pull request here, but in truth, all my commits (and @mmha 's commit too, and a few from you) is included in this PR, along with some merge commits. If this pull request is only the experimental include fix, shouldn't it just be a single commit, the cherry-pick of 0b201f8? |
|
Yes, you are right. I cherry-picked your whole pull request. I see three options:
|
|
You can also fix this PR by (with your unify_experimantal_includes branch checked out locally): |
Starting with GCC 10.1, GCC/libstdc++ supports coroutines, but includes them in their final location in <coroutine>. This commit generalizes the location of the header, and the name of the types (either in the std or std::experimental namespace), so that both the current Clang/MSVC and the GCC approach can be supported. This should also guarantee that both current and later Clang/MSVC releases will be supported by the library.
andreasbuhr
force-pushed
the
unify_experimental_includes
branch
from
48a7b99 to
b62a8af
Compare
The change for <coroutine> vs. <experimental/coroutine> is replicated for <filesystem> in this patch. In addition, the std::experimental namespace is replaced by cppcoro:: namespace in a few more places.
andreasbuhr
force-pushed
the
unify_experimental_includes
branch
from
b62a8af to
06b3bab
Compare
lewissbaker
left a comment
lewissbaker
left a comment
There was a problem hiding this comment.
Thanks! A couple of minor comments.
But otherwise looks good.
include/cppcoro/coroutine.hpp
Outdated
|
|
||
| using suspend_always = std::suspend_always; | ||
| using suspend_never = std::suspend_never; | ||
| static inline auto noop_coroutine() { return std::noop_coroutine(); } |
There was a problem hiding this comment.
Can this just be using std::noop_coroutine;?
Similarly for the other names?
using std::coroutine_handle;
using std::suspend_always;
using std::suspend_never;
using std::noop_coroutine;
There was a problem hiding this comment.
yes, this looks much better. Done.
| #include <atomic> | ||
| #include <optional> | ||
| #include <experimental/coroutine> | ||
| #include <cppcoro/coroutine.hpp> |
There was a problem hiding this comment.
Maybe remove this include if it's not being used here?
| #include <atomic> | ||
| #include <optional> | ||
| #include <experimental/coroutine> | ||
| #include <cppcoro/coroutine.hpp> |
There was a problem hiding this comment.
Similarly here. Remove this include?
| # include <atomic> | ||
| # include <optional> | ||
| # include <experimental/coroutine> | ||
| # include <cppcoro/coroutine.hpp> |
There was a problem hiding this comment.
Remove this include? It doesn't seem to be used.
I'll just squash the changes during the merge. |
lewissbaker
left a comment
lewissbaker
left a comment
There was a problem hiding this comment.
Thanks for making those changes.
include/cppcoro/coroutine.hpp
Outdated
| using std::experimental::coroutine_handle; | ||
| using std::experimental::suspend_always; | ||
| using std::experimental::suspend_never; | ||
| using std::experimental::noop_coroutine; |
There was a problem hiding this comment.
This seems to break MSVC 2017 (and I think also some MSVC 2019 versions), which doesn't have noop_coroutine().
Maybe put this inside #if CPPCORO_COMPILER_SUPPORTS_SYMMETRIC_TRANSFER and add an include of <cppcoro/config.hpp>?
|
This pull request is now open for two month. I now stop maintaining this pull request. From now on, I will continue to maintain the master branch in https://github.com/andreasbuhr/cppcoro. There we have CMake support, a CI based on Github Actions, and support for the latest compilers. |
4 participants
This pull request cares about three issues:
#include <coroutine>vs.#include <experimental/coroutine>problem. A header is introduced which checks which include is available.std::vs.std::experimental::namespace issue. Coroutine related things a defined into the cppcoro:: namespace.This pull request is based on the pull request #158 by @dutow. Thanks to @dutow for the great work.
Pull request 158 addresses several issues at the same time. I extracted this pull request which cares only about one single issue, in order to have an easy to review pull request.
To help improve this pull request, please fork https://github.com/andreasbuhr/cppcoro/tree/unify_experimental_includes and create a pull request toward this branch.