Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify experimental includes #171

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

andreasbuhr
Copy link
Contributor

@andreasbuhr andreasbuhr commented Oct 10, 2020

This pull request cares about three issues:

  1. The #include <coroutine> vs. #include <experimental/coroutine> problem. A header is introduced which checks which include is available.
  2. The std:: vs. std::experimental:: namespace issue. Coroutine related things a defined into the cppcoro:: namespace.
  3. The same for filesystem vs experimental/filesystem

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.

@dutow
Copy link

dutow commented Oct 10, 2020

@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?

@andreasbuhr
Copy link
Contributor Author

Yes, you are right. I cherry-picked your whole pull request. I see three options:

  1. Leave it like this, accepting clutter in the history.
  2. Recreating this pull request as a cherry-pick of 0b201f8
  3. Leave it like this, squashing it into one commit on merge.
    What would you prefer?

@dutow
Copy link

dutow commented Oct 10, 2020

You can also fix this PR by (with your unify_experimantal_includes branch checked out locally):

git reset --hard master
git cherry-pick 0b201f8
git push --force-with-lease

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.
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.
Copy link
Owner

@lewissbaker lewissbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A couple of minor comments.
But otherwise looks good.


using suspend_always = std::suspend_always;
using suspend_never = std::suspend_never;
static inline auto noop_coroutine() { return std::noop_coroutine(); }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this looks much better. Done.

@@ -11,7 +11,7 @@

#include <atomic>
#include <optional>
#include <experimental/coroutine>
#include <cppcoro/coroutine.hpp>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove this include if it's not being used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -11,7 +11,7 @@

#include <atomic>
#include <optional>
#include <experimental/coroutine>
#include <cppcoro/coroutine.hpp>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here. Remove this include?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -15,7 +15,7 @@

# include <atomic>
# include <optional>
# include <experimental/coroutine>
# include <cppcoro/coroutine.hpp>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this include? It doesn't seem to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@lewissbaker
Copy link
Owner

I see three options:

  1. Leave it like this, accepting clutter in the history.
  2. Recreating this pull request as a cherry-pick of 0b201f8
  3. Leave it like this, squashing it into one commit on merge.
    What would you prefer?

I'll just squash the changes during the merge.

Copy link
Owner

@lewissbaker lewissbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes.

using std::experimental::coroutine_handle;
using std::experimental::suspend_always;
using std::experimental::suspend_never;
using std::experimental::noop_coroutine;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed now thx to @Garcia6l20

@andreasbuhr andreasbuhr marked this pull request as ready for review November 4, 2020 21:28
@andreasbuhr
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants