Skip to content

Replace const std::vector& with boost::span#50

Open
vsoulgard wants to merge 2 commits intoboostorg:developfrom
vsoulgard:feature/49-use-boost-span
Open

Replace const std::vector& with boost::span#50
vsoulgard wants to merge 2 commits intoboostorg:developfrom
vsoulgard:feature/49-use-boost-span

Conversation

@vsoulgard
Copy link
Copy Markdown
Contributor

close #49

Allows to accept any contiguous memory (arrays, vectors, or raw buffers) without being tied to a specific storage type.

But boost::span does not construct from initializer_list.

@biljazovic
Copy link
Copy Markdown
Collaborator

Hi, thanks for the PR!

Issue is that this code would stop working (heap-use-after-free)

auto op = c.async_subscribe(
    std::vector<subscribe_topic>{{ "test/mqtt-test", { qos_e::exactly_once } }}, subscribe_props {},
    asio::deferred
);
/* initiate op later */

In the current implementation, the vector is copied only if the initiation is deferred; otherwise it's forwarded by reference (async_initiate takes care of that).

I think the correct solution is to use boost::span in (un)subscribe_op like you do, and modify async_(un)subscribe like this:

template <
    typename TopicSequence,
    typename CompletionToken =
        typename asio::default_completion_token<executor_type>::type
>
decltype(auto) async_subscribe(
    const TopicSequence& topics,
    const subscribe_props& props,
    CompletionToken&& token = {}
) {
    using Signature = void (
        error_code, std::vector<reason_code>, suback_props
    );
    return asio::async_initiate<CompletionToken, Signature>(
        detail::initiate_async_subscribe(_impl), token,
        topics, props
    );
}

Regarding std::initializer_list, we could add an overload:

template <
    typename CompletionToken =
        typename asio::default_completion_token<executor_type>::type
>
decltype(auto) async_subscribe(
    std::initializer_list<subscribe_topic> topics,
    const subscribe_props& props,
    CompletionToken&& token = {}
) {
    return async_subscribe(std::vector(topics), props, std::forward<CompletionToken>(token));
}

@vsoulgard
Copy link
Copy Markdown
Contributor Author

Thank you for the detailed review and clear guidance!

I've comitted new changes. For async_(un)subscribe, I had to add new trait and use typename = std::enable_if_t<detail::is_container_of<TopicSequence, std::string>> to properly constrain the template. I don't know if there is better way to do this.

@biljazovic
Copy link
Copy Markdown
Collaborator

Thanks for the update! I'd like to explore a few options for constraining the template before merging. I'll get back to you on this.

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.

Use boost::span instead of passing std::vector by ref

2 participants