Unify experimental includes #171

andreasbuhr · 2020-10-10T12:22:25Z

This pull request cares about three issues:

The #include <coroutine> vs. #include <experimental/coroutine> problem. A header is introduced which checks which include is available.
The std:: vs. std::experimental:: namespace issue. Coroutine related things a defined into the cppcoro:: namespace.
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 · 2020-10-10T12:30:20Z

@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 · 2020-10-10T12:34:57Z

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

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

dutow · 2020-10-10T12:38:45Z

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.

…includes

lewissbaker

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

lewissbaker · 2020-10-12T22:35:05Z

include/cppcoro/coroutine.hpp

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


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;

yes, this looks much better. Done.

lewissbaker · 2020-10-12T22:41:43Z

include/cppcoro/file_read_operation.hpp

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

 #include <atomic>
 #include <optional>
-#include <experimental/coroutine>
+#include <cppcoro/coroutine.hpp>


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

lewissbaker · 2020-10-12T22:42:29Z

include/cppcoro/file_write_operation.hpp

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

 #include <atomic>
 #include <optional>
-#include <experimental/coroutine>
+#include <cppcoro/coroutine.hpp>


Similarly here. Remove this include?

lewissbaker · 2020-10-12T22:44:12Z

include/cppcoro/net/socket_accept_operation.hpp

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

 # include <atomic>
 # include <optional>
-# include <experimental/coroutine>
+# include <cppcoro/coroutine.hpp>


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

lewissbaker · 2020-10-13T00:10:15Z

I see three options:

Leave it like this, accepting clutter in the history.

Recreating this pull request as a cherry-pick of 0b201f8

Leave it like this, squashing it into one commit on merge.
What would you prefer?

I'll just squash the changes during the merge.

lewissbaker

Thanks for making those changes.

lewissbaker · 2020-10-13T06:00:47Z

include/cppcoro/coroutine.hpp

+  using std::experimental::coroutine_handle;
+  using std::experimental::suspend_always;
+  using std::experimental::suspend_never;
+  using std::experimental::noop_coroutine;


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

fixed now thx to @Garcia6l20

Unify experimental includes

andreasbuhr · 2020-12-06T09:01:45Z

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.

andreasbuhr force-pushed the unify_experimental_includes branch from 48a7b99 to b62a8af Compare October 10, 2020 12:55

Also support <filesystem> as well as <experimental/filesystem>

06b3bab

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 October 10, 2020 20:57

andreasbuhr mentioned this pull request Oct 11, 2020

cppcoro development seems pretty dead #170

Open

Merge remote-tracking branch 'origin/master' into unify_experimental_…

81e9133

…includes

lewissbaker requested changes Oct 13, 2020

View reviewed changes

andreasbuhr added 2 commits October 13, 2020 02:24

simplify coroutine.hpp header

81708c2

remove unused includes

0550a5d

lewissbaker approved these changes Oct 13, 2020

View reviewed changes

lewissbaker reviewed Oct 13, 2020

View reviewed changes

andreasbuhr mentioned this pull request Oct 22, 2020

add MSVC 2019 x64 build andreasbuhr/cppcoro#16

Merged

Garcia6l20 and others added 6 commits October 23, 2020 07:48

fix unknown noop_coroutine type

25c811e

no MSVC 2017 workarounds on MSVC 2019

3d6cd95

Merge pull request #17 from Garcia6l20/unify_experimental_includes

016b67b

Unify experimental includes

fixed VS 2017 builds

1081167

fix test for MSVC 2017

d23ce4d

fix test for MSVC 2017

f71a7f6

andreasbuhr marked this pull request as ready for review November 4, 2020 21:28

cathaysia mentioned this pull request Jan 29, 2021

add cppcoro library (header only) msys2/MINGW-packages#7834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify experimental includes #171

Unify experimental includes #171

andreasbuhr commented Oct 10, 2020 •

edited by lewissbaker

Loading

dutow commented Oct 10, 2020

andreasbuhr commented Oct 10, 2020

dutow commented Oct 10, 2020

lewissbaker left a comment

lewissbaker Oct 12, 2020

andreasbuhr Oct 13, 2020

lewissbaker Oct 12, 2020

andreasbuhr Oct 13, 2020

lewissbaker Oct 12, 2020

andreasbuhr Oct 13, 2020

lewissbaker Oct 12, 2020

andreasbuhr Oct 13, 2020

lewissbaker commented Oct 13, 2020

lewissbaker left a comment

lewissbaker Oct 13, 2020

andreasbuhr Oct 23, 2020

andreasbuhr commented Dec 6, 2020

Unify experimental includes #171

Are you sure you want to change the base?

Unify experimental includes #171

Conversation

andreasbuhr commented Oct 10, 2020 • edited by lewissbaker Loading

dutow commented Oct 10, 2020

andreasbuhr commented Oct 10, 2020

dutow commented Oct 10, 2020

lewissbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewissbaker commented Oct 13, 2020

lewissbaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasbuhr commented Dec 6, 2020

andreasbuhr commented Oct 10, 2020 •

edited by lewissbaker

Loading