Skip to content

Extended tasks so they can return references and not just values.#154

Merged
jbaldwin merged 2 commits intojbaldwin:mainfrom
bruno-j-nicoletti:dev/bruno/tasksReturningReferences
Jun 17, 2023
Merged

Extended tasks so they can return references and not just values.#154
jbaldwin merged 2 commits intojbaldwin:mainfrom
bruno-j-nicoletti:dev/bruno/tasksReturningReferences

Conversation

@bruno-j-nicoletti
Copy link
Contributor

I needed the ability to return a reference to an object from a task, rather than copying the object. So I extended tasks to do that and added two tests for it.

There seems to be some reformatting spam in task.h from clang-format being run.

@jbaldwin
Copy link
Owner

For your use case can you return a value type that wraps the ref? My worry here by allowing users to return ref types is that it becomes extremely easy to return a dangling reference on the coroutine frame which is immediately invalid upon the task completing. It's certainly still possible to do so today but it's a bit harder.

I see in one of the tests it is returning a ref to an object that is created before the coroutine which is technically a valid case, but I think it could be worked around by introducing a small wrapper class to the red, e.g.

struct Wrapper
{
    int& result
}

Would this work for your use case?

@bruno-j-nicoletti
Copy link
Contributor Author

I can think of two ways to have a task return a reference to an object with the current API, either via a pointer or as a reference wrapper inside an optional...

using MyRefTask = coro::task<std::optional<std::reference_wrapper<MyType>>;
using MyPtrTask = coro::task<MyType *>;

The first case is kinda ugly from the users point of view, as it requires you to handle the optional being returned by the task. You have to put the std::reference_wrapper into a std::optional, as otherwise the promise that contains it can't be constructed.

My PR effectively implements that, but pushes it into the library. Which reminds me, it should use std::optional::value in the promise's 'result' function, rather than the dereference operator so an unset optional will throw an exception.

I'd argue that if a user of your library has been explicit in saying a task returns a reference, then they know what they are doing. You have already given the users the ability to create dangling references by letting tasks return pointers.

With respect to the test case, it reflects the use case I envisage, which is a task that returns a reference to an object that has a lifetime that spans the coroutine. In my case it is a value held in a persistent data cache that the tasks are populating.

@jbaldwin
Copy link
Owner

Yeah I'm not opposed to it, just figured it wasn't necessary a long time ago since there are ways around it but there are certainly good reasons to do it and it is a part of the language. Like anything else with c++ user beware... And using std::reference_wrapper is a typing pain too, it's so long!

@jbaldwin jbaldwin merged commit a313694 into jbaldwin:main Jun 17, 2023
dok-net added a commit to dok-net/libcoro that referenced this pull request Jun 21, 2023
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.

3 participants