Croutons in kdereview
Ivan Čukić
ivan.cukic at kde.org
Sun Aug 29 12:31:12 BST 2021
Hi,
Cool to have a library like this.
I have a few nit-picks.
I'd put the whole library in a namespace as it uses some quite common names
like Result, Error and Future.
future.h:15
The Result type could be made more API-compatible with the expected
std::expected. Things like explicit operator bool() const noexcept; s/ok/
has_value/, and others. See [1]
Also, you should probably add a static_assert(not std::is_same_v<T, E>)
because of holds_alternative and std::get.
Alternatively, you can use index-based checks and get.
The value and error member functions should return const& to T and Error
respectively instead of forcing a copy. (if you want a bit more than this,
you can add && overloads that return T&& and Error&&).
future.h:31
Replace is_same...::value with is_same_v for readability
future.h:119
Is it needed to have a std::function there (having something wrapped into
std::function and then that std::function wrapped in a lambda) - why not make
the then member function a function template? (in other places as well)
Also, naming both the function and the argument `then` is a bit icky.
coroutine_integration.h:12
An overly common namespace name/alias `ns` exported by this header.
coroutine_integration.h:37
t is default-constructed and then assigned a new default-constructed instance.
Any reason for this? (same issue in other constructors in this file) What's
more, most constructors in this file do not seem to serve a purpose.
Cheers,
Ivan
[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/
p0323r8.html#expected.synop
--
dr Ivan Čukić
ivan at cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232 07AE 01C6 CE2B FF04 1C12
More information about the kde-core-devel
mailing list