Thank you, Andy, for the review. Stjepan On Mon, Nov 10, 2008 at 9:18 AM, Andrew Webber <andy@aligature.com> wrote:
For this review, I have focused on the usability and documentation of the new signals2 library. I haven't really probed into the implementation and so I'm assuming that the code is adequate and mostly bug free.
In general, I think that this library should be accepted as a boost library. Other than a few questions and annoyances, I think the library is an extremely welcome addition to the boost libraries.
I think that the interface changes required to guarantee thread safety are understandable. I've seen some discussion on the mailing list about retaining the trackable interface for backwards compatibility. While I understand the desire to keep the changes required by switching to signals2 to a minimum, I think signals2 provides a clean break. I understand boost::signals is going to be maintained for the time being. This way, if people don't want to change, they don't need to for the time being. If they want thread safe signals, they should switch to the new library, its new techniques and its clean interface.
Here are my specific comments: * Why the change to boost::optional for signal return values. Maybe I missed a discussion about the motivation on the mailling list, but this is an interface change that appears to be unnecessary.
* I would like to see examples provided to explain the use of postconstructable, predestructable, and deconstruct_ptr. I think a concrete example might clear up some people's confusion over when it's appropriate to use these.
* I'd also like to see a concrete example shown of switching out the mutex class used. I think three examples would be best: built-in, dummy_mutex, and boost::mutex.
* I'm not very happy about the tendancy of the slot disconnect exceptions to show up in Visual Studio output. For instance, I built a test program where several tracked objects were destroyed from different threads, based on a random timer, while the slot was being invoked over and over. The program worked well except for a great deal of repeated output like the following:
First-chance exception at 0x7c812a5b in SignalsTest.exe: Microsoft C++ exception: boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::bad_weak_ptr>
at memory location 0x0012dc0c.. First-chance exception at 0x7c812a5b in SignalsTest.exe: Microsoft C++ exception: boost::signals2::expired_slot at memory location 0x0012df58..
I did several experiments with this library on Microsoft Visual Studio 2005. I did not encounter any bugs or problems during my testing. I have a decent amount of experience in this problem domain. In the past, I have implemented a (limited) thread safe wrapper around the boost::signals library. The signals2 library would very effectively replace my wrapper. I spent a couple of hours on my evaluation, I would consider it moderately in-depth without evaluating the signals2 implementation.
Best, Andy Webber