Croutons in kdereview

Ivan Čukić ivan.cukic at
Sun Aug 29 12:31:12 BST 2021


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.


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&&).


Replace is_same...::value with is_same_v for readability


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.


An overly common namespace name/alias `ns` exported by this header.


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.



dr Ivan Čukić
ivan at,
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

More information about the kde-core-devel mailing list