Asio recipe update - Githubissues

cstratopoulos commented 4 years ago

Fixes #195

Sorry for the delay, have been busy but also in ways that have resulted in further improvements to the recipe gist.

Opening this PR for preliminary feedback from you on the prose and code style. I've tried to follow your lead of course. The new form of async_result is an interesting and somewhat foundational departure from the old one. I tried to strike a balance in communicating why it has to look the way it does without getting into too much detail on the machinery involved. See https://github.com/cstratopoulos/outcome/commit/d1324f9cf9008da47d8d98323c10d7a6937f78be for the diff where I added a bit more motivation.

The thing I tried to hand-wave a bit is that, strictly speaking, it would be incorrect to say "our async_result calls the initiate method of an underlying async_result" because the underlying async_result may very well have the old form with a get method; it handles both cases by using the async_initiate helper which follows the example set by redirect_error. This is where I got the wording

it will pass the initiation work off to an async_result for the supplied completion token type with a completion handler which consumes result<T>....

In practice, however, will anybody be using as_result atop an async_result which doesn't have an initiate method? ....seems unlikely ¯(ツ)/¯

I've got it compiling fine on my machine for what that's worth. However I'm not sure what you'll want to do with the #if 0 debug block in the final snippet. As I alluded above and in #195, the new async_result means we can't guarantee the existence of the get method nor the completion_handler_type type.

ned14 commented 4 years ago

Firstly, thanks for the PR.

Reading through this, I guess we have a choice to make before progressing: Should we split the recipe into two, one for Boost < 1.70 and another for Boost >= 1.70, or persist with trying to keep one recipe which works on both? I've not touched ASIO for two years now, so I cannot have a useful opinon here. It's whatever you think is best.

cstratopoulos commented 4 years ago

Hmm tough call. I guess your concern is for standalone outcome users who are stuck with older Boost versions?

My suggestion, and what I think the easiest thing might be, is to remark at the beginning of the recipe that ASIO's async_result and coroutines support changed with 1.70 and then, since old versions of the Boost website/docs live forever, it might be easier to permalink to

the boost.org 1_70_0 version of the ASIO recipe discussion docs, and
the last git gist commit before I switched to the new version, since the existing hyperlink/embed points to the latest version.

This might just be an afternoon's work, but I suspect if we reaaally wanted, it would be possible to write a single async_result that would work drop-in with Boost < 1.70 and >= 1.70 but it would get kind of ugly.

Also a remark on support/compatibility. When it comes to composing as_result with other tokens, that's kind of the nice thing about this new initiate form of async_result: the async_initiate function is provided precisely as a backwards compatibility shim for the old form of async_result with a get method and completion_handler_type: it will enable_if resolve to either the initiate method or the old get method. That's why I've implemented the initiate method in terms of async_initiate. So someone could, for example (in theory; haven't tried), use this new recipe form with the stackful coro yield_context completion token which hasn't changed much in a few boost versions and still has an async_result with a get method.

So in other words, the ugliness I alluded to above would involve essentially duplicating the backwards compatibility logic that Chris put into creating async_initiate.

If you'll allow some wishful thinking/observations, I think all my implementations in as_result.hpp which mention outcome::result directly could instead look like

template<
    class CompletionToken,  
    // could SFINAE concept check if wanted
    template<class...> class ValueOrErrorType> 
struct basic_as_result_t;

template<class CompletionToken>
using as_result_t = basic_as_result_t<CompletionToken, outcome::result>;

and similarly for detail::outcome_result_handler, detail::outcome_result_signature, etc.

And now that Outcome is a part of Boost I wonder if it wouldn't be unreasonable to wish for Outcome ASIO support to live in ASIO as is the case, e.g., with the Boost.Coroutine integration. I had actually shared an early as_result.hpp with Chris when he was first working on the this_coro::token stuff way back when, but I know we probably don't want to open the "submitting PRs to ASIO" can of worms here

ned14 commented 4 years ago

I was more thinking of splitting the recipe into a pre-1.71 edition, and a post-1.71 edition?

cstratopoulos commented 4 years ago

Makes sense to me. Do you have a layout/structure in mind for how best to present the two recipes? There's a fair bit of shared material (the token/factory function and the handler), with divergence around async_result and the form of the completion token, which I suppose affects the example source code too.

ned14 commented 4 years ago

I'd just fork the page. Leave existing page alone, make new page by copy & paste. Change titles appropriately.

cstratopoulos commented 4 years ago

I pushed updates with the forked version as you suggested.

Some remarks, feel free to veto/delete as you see fit of course.

The version split number I used is 1.70 rather than 1.71 which is correct from the point of view of version compatibility (i.e., someone on Boost 1.67-1.69 needs the old recipe), but not sure if you'd prefer to split according to the Boost release where the recipe was updated (i.e., we're forking the recipe for version 1.71)
I added a compatibility note section where each recipe links to the other. I kept it brief but figure this could be useful for less observant readers or those who land at a single page by permalink
I also forked the snippets cpp file--even though we just extract snippets to embed in the documentation I figure it's desirable if the files could be compiled on their own still

Also as per https://github.com/ned14/outcome/issues/199 I've been using/testing an OUTCOME_CO_TRY in my local copy of the ASIO recipe. Things seem to work fine but the macro overloading business is new to me so I wanted to make sure I haven't made any glaring oversights in adapting it:

#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ >= 8
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wparentheses"
#endif

#define BOOST_OUTCOME_CO_TRYV2(unique, ...)                                                                                                                                                                                                                                                                                             \
  auto && (unique) = (__VA_ARGS__);                                                                                                                                                                                                                                                                                            \
  if(!(unique).has_value())                                                                                                                                                                                                                                                                                                    \
  co_return BOOST_OUTCOME_V2_NAMESPACE::try_operation_return_as(static_cast<decltype(unique) &&>(unique))
#define BOOST_OUTCOME_CO_TRY2(unique, v, ...)                                                                                                                                                                                                                                                                                           \
  BOOST_OUTCOME_CO_TRYV2(unique, __VA_ARGS__);                                                                                                                                                                                                                                                                                          \
  auto && (v) = BOOST_OUTCOME_V2_NAMESPACE::detail::try_extract_value(static_cast<decltype(unique) &&>(unique))

#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ >= 8
#pragma GCC diagnostic pop
#endif

#define BOOST_OUTCOME_CO_TRYV(...) BOOST_OUTCOME_CO_TRYV2(BOOST_OUTCOME_TRY_UNIQUE_NAME, __VA_ARGS__)

#define BOOST_OUTCOME_CO_TRYA(v, ...) BOOST_OUTCOME_CO_TRY2(BOOST_OUTCOME_CO_TRY_UNIQUE_NAME, v, __VA_ARGS__)

#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY8(a, b, c, d, e, f, g, h) BOOST_OUTCOME_CO_TRYA(a, b, c, d, e, f, g, h)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY7(a, b, c, d, e, f, g) BOOST_OUTCOME_CO_TRYA(a, b, c, d, e, f, g)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY6(a, b, c, d, e, f) BOOST_OUTCOME_CO_TRYA(a, b, c, d, e, f)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY5(a, b, c, d, e) BOOST_OUTCOME_CO_TRYA(a, b, c, d, e)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY4(a, b, c, d) BOOST_OUTCOME_CO_TRYA(a, b, c, d)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY3(a, b, c) BOOST_OUTCOME_CO_TRYA(a, b, c)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY2(a, b) BOOST_OUTCOME_CO_TRYA(a, b)
#define BOOST_OUTCOME_CO_TRY_INVOKE_TRY1(a) BOOST_OUTCOME_CO_TRYV(a)

#define BOOST_OUTCOME_CO_TRY(...) BOOST_OUTCOME_TRY_CALL_OVERLOAD(BOOST_OUTCOME_CO_TRY_INVOKE_TRY, __VA_ARGS__)

If that looks alright I'll push it to the git gist and I can add an additional snippet/discussion demonstrating its use.

ned14 commented 4 years ago

Ok, merged. Thanks for the contribution.

FYI, I intend to add coroutine TRY directly into Outcome. It won't be quite your implementation, but I'll get to it when I get the time to merge the Coroutines build infrastructure from LLFIO. I'm currently blocked on the Boost release for that.

cstratopoulos commented 4 years ago

Hey sorry one last thing (hopefully), I just looked at the regenerated boostorg.github.io docs from the last merge. For both recipes the the gist link is https://gist.github.com/cstratopoulos/901b5cdd41d07c6ce6d83798b09ecf9b

This automatically serves the latest version, but for < 1.70 this is the last revision that still used asio::experimental https://gist.github.com/cstratopoulos/901b5cdd41d07c6ce6d83798b09ecf9b/da584844f58353915dc2600fba959813f793b456

Maybe I can leave this last bit to you? I don't have hugo set up locally, so I'm not sure if their gist command supports pointing to specific revisions, i.e., something like

-{{% gist "cstratopoulos" "901b5cdd41d07c6ce6d83798b09ecf9b" %}}
+{{% gist "cstratopoulos" "901b5cdd41d07c6ce6d83798b09ecf9b/da584844f58353915dc2600fba959813f793b456" %}}

ned14 / outcome

Asio recipe update #198