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