waitFor(QCoro::Task<T>): Do not require T to be default constructible#223
Conversation
danvratil
left a comment
There was a problem hiding this comment.
Thanks a lot for the patch! Could you please update the patch and drop the branch for default-constructible types and just always use std::optional? It actually is the more correct approach since we avoid the (potentially high) cost of calling the default constructor, which also avoids the triggering of any potential side-effects that the constructor might have.
Could you also please add a unit-test for a non-default-constructible type to make sure we don't regress in the future? Thank you
|
Uses the |
danvratil
left a comment
There was a problem hiding this comment.
The MacOS CI is lazy today for some reason, but the change passes on Linux Clang and I don't have any reason to suspect AppleClang to not pass as well here.
Thanks a lot for the change and for adding the unittest 👍.
There's one more PR waiting, once it's merged I'll do a new QCoro release with those changes.
A patch so that
Tno longer needs to be default constructible when callingQCoro::waitFor(QCoro::Task<T>). Not claiming that this is the best way to do things, but it worked for me. That said I would appreciate any additional testing. I'm only a light user of the QCoro library.I ended up duplicating some code logic, because I'm not sure if using
std::optionalin all cases could decrease performance. Previously aTwould always be default constructed, but then RVO should occur when returning the value fromdetail::waitFor()so no additional copy/move would occur. If the default constructor forTis cheap, this should have been fine.With using a
std::optional<T>though,Ts default constructor is never called, but I don't think RVO can occur anymore with returning its value fromdetail::waitFor(). I thinkTmay be moved to return its value, but this is a corner of C++ I don't have a complete grasp on.closes #222