On 20 Jun 2015 at 13:48, Bjorn Reese wrote:
On 06/19/2015 07:03 PM, Niall Douglas wrote:
https://github.com/ned14/boost.spinlock/blob/master/include/boost/spinlock/m...
This starts with the words "The world's most simple C++ monad" followed by 1200 lines of code... makes one scared of monads.
Firstly, thanks for the review. As I know you know, STL quality C++ is amazingly verbose. And I haven't implemented monad<void> yet, which will require doubling the existing lines of code to stamp out an almost identical monad<void> specialisation. I am currently strongly considering an automatically generated source solution to that. (One could use enable_if everywhere to get a void and non-void single implementation, but I want minimal build time costs).
Here are some random throughts I had while reading the code.
Minor comment, consider letting monad_error inherit from system_error instead of logic_error.
http://en.cppreference.com/w/cpp/thread/future_error I agree with you, but that's the C++ standard. Of course, monad need not duplicate future here, but I think if I claim "future semantics" then it ought to.
When throwing monad_error, you cast to int (presumably to satisfy some compiler.) Can't you simply use make_error_code() instead? That will also ensure that you set the correct category.
VS2015 RC was the problem. The current cast to int workaround is the first thing I'll try fixing when VS2015 RTM comes out. The category is correct though. The throw_error() function is a template parameter, and that is a struct detail::throw_monad_error which in turn throws monad_error(monad_errc). monad_error takes a std::error_code, and the C++ 11 STL uses the std::is_error_code_enum<> trait to auto convert the monad_errc enum into the correct error code and monad_category category. I lifted that pattern of how to throw a future_error straight from Boost and two STLs I examined. It's identical to std::future<T> and boost::future<T>.
Why does one of the value_storage constructors call abort() rather than throwing an exception?
value_storage lives in the detail namespace, and its ability to carry a future<T>* in its variant space is an internal implementation detail nor exposed to public users. The only users of that facility are lightweight future-promise. They (should) never ever copy construct a value_storage, so if a copy construction ever occurs when the contents are a future<T>*, that's memory corruption and/or bad programming by me. It was purely defensive programming on my part.
Why does monad::get_error() throw on no_state, rather than simply returning an error_code that says so?
A very good question. I purely did it that way round for these two reasons: 1. Any attempt to get any state from an empty monad always throws no_state consistenty no matter what you call. 2. future-promise always throws a no_state if either has no state, so boost::future<T>::get_exception_ptr() will throw a no_state instead of returning an exception_ptr of no_state. monad<T> matches that behaviour. Actually, I just lied. The monad get_or(), get_error_or(), get_exception_or() functions never throw no_state, they return the OR value. I am unsure if that is a wise design choice. There is also another potential design problem in that get_error() returns a null error_code if the monad is not errored even if it is excepted. I did it that way to match get_exception() as boost::future<T>::get_exception_ptr() returns a null exception_ptr if there is no exception. However there is a discrepency, because if the monad is errored not excepted and you call get_exception(), you'll get an exception_ptr to the system_error for the error_code, and it's not the same the other way round. There is a logic to this though. monad<T> is effectively a tribool state of empty/value/excepted where an errored state always converts when needed into an exception state, but never the other way round. The errored state is therefore a non-type-erased excepted state, while the excepted state is a type-erased state. I am just unsure if that is a wise design.
Is there any difference between monad::get() and monad::value()? If they are synonyms, then you may consider removing monad::get() to keep the class interface smaller, and instead overload std/boost::get().
They are identical.
I hate multiple names for the same function. However, Expected