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

less use of auto in examples #2931

Open
vinniefalco opened this issue Sep 19, 2024 · 7 comments
Open

less use of auto in examples #2931

vinniefalco opened this issue Sep 19, 2024 · 7 comments

Comments

@vinniefalco
Copy link
Member

Examples should not use auto when showing how to do important things, such as here:

https://github.com/boostorg/beast/blob/develop/example/http/server/awaitable/http_server_awaitable.cpp#L253

@sehe
Copy link
Collaborator

sehe commented Sep 19, 2024

TBF I think the auto ex = co_await this_coro::executor; should not be "improved", since it hides the fact that the awaitable can be instantiated with a specific executor type. In the simplistic approach of just spelling out asio::any_io_executor it causes (a) people not to notice it's actually a dependent type (b) automatic create the habit of performance pessimization by creating the type-erasure and ref-counting overhead associated with it.

Yes, the auto x = T{a,b,c}; instead of just T X {a,b,c}; can die. I feel that's a cosmetic trend on the same level as aligning assignments or declaration (it's the only appeal).

@vinniefalco
Copy link
Member Author

I want to see the type in the example though. Is it using any_io_executor? I don't know. I would like to. Although I don't want to construct one needlessly... what is the actual type?

@sehe
Copy link
Collaborator

sehe commented Sep 19, 2024

Yeah that's the thing. It's actually decltype(co_await this_coro::executor). It depends on the awaitable/promise. And that's encoded in the coro return type (thank C++ standards). You really do not want to know the type here. It's bad enough that the default be any_io_executor. No need to instill that as an inescapable pit-of-doom in the examples. If you must, create a global typedef for it?

// using Executor = asio::any_io_executor;
using Executor = asio::thread_pool::executor_type;

asio::awaitable<void, Executor> my_coro() {
    Executor      my_executor = co_await asio::this_coro::executor;
    tcp::acceptor acc(my_executor, {{}, 8989});
    while (true) {
        auto strand = make_strand(my_executor);
        tcp::socket sock   = co_await acc.async_accept(strand /*, asio::deferred*/);
        asio::co_spawn(strand, my_session(std::move(sock)), asio::detached);
    }
}

//     asio::thread_pool ioc(3);
//     asio::co_spawn(ioc, my_coro(), asio::detached);

However, looking one step further along that road, you would get things like

asio::awaitable<void, asio::strand<Executor>> my_session(tcp::socket socket) {
    asio::strand<Executor> my_executor = co_await asio::this_coro::executor;
    // TODO
    co_return;
}

This is why I think it's splendid idiomatic code to deduce the local executor type from the coro's promise.

@vinniefalco
Copy link
Member Author

Yes I see your point. I agree that it is idiomatic, but example code sometimes needs to prioritize explanatory power over the use of idioms. In this case I want the reader to know whether or not type-erasure is happening in this particular example. Naming the type for the variable has problems, but how about instead we prefix that line with a static_assert assuring that we are getting the type we expected? This shows the reader what is happening in a way that preserves the idiom.

@sehe
Copy link
Collaborator

sehe commented Sep 19, 2024

Marginally better. Perhaps with a message stating that the executor type is the one declared in the return value's promise type.

static_assert(std::is_same_v<decltype(ex), asio::any_io_executor>, "by default asio::awaitable<T> uses the type-erasing executor container");

I truly think stuff of this kind ought to be documented in their place, like here https://www.boost.org/doc/libs/1_86_0/doc/html/boost_asio/reference/this_coro__executor.html

In short, I think there's introductory documentation (tutorial style) that should explain these things. Once. Not something to contaminate all examples with. Instead, I expect the examples to show good practice, idiomatic code.

@vinniefalco
Copy link
Member Author

I expect the examples to show good practice, idiomatic code.

That is normally the case, but it is surprising to discover that type-erasing is taking place here. Hiding it is not so great. And you can't expect users to read the entire manual before using the examples.

@sehe
Copy link
Collaborator

sehe commented Sep 19, 2024

Oh I hear you. Like said before "It's bad enough that the default be any_io_executor. No need to instill that as an inescapable pit-of-doom in the examples". I'd love some highlevel documentation about performance considerations of the default executor type.

In other words, while I agree that people need to be instructed/educated about this, I disagree that it should be recurring in all examples.

For balance: as an example of a recurring piece of explanation that I do appreciate is this comment:

    void
    run()
    {
        // We need to be executing within a strand to perform async operations
        // on the I/O objects in this session. Although not strictly necessary
        // for single-threaded contexts, this example code is written to be
        // thread-safe by default.
        net::dispatch(ws_.get_executor(),
            beast::bind_front_handler(
                &session::on_run,
                shared_from_this()));
    }

The reason is that it pre-empts common confusions/oversight by people learning from this example. It does not easily lead to significantly sub-optimal code, as opposed to hard-coding any_io_executor.

In the case of co_await this_core::executor, people who are curious enough will look at the documentation of it. I say, THAT needs to be improved, if only by linking to the Overview article explaining C++20 coroutines in Asio.

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

No branches or pull requests

3 participants
@sehe @vinniefalco and others