Pavel Vozenilek wrote:
Review of Finite State Machine library written by Andreas Huber starts today, February 21, and lasts 10 days.
I hope it's not too late to submit a review.
* What is your evaluation of the documentation? How easy (or hard) it is to understand library features? What can be improved?
First time I read tutorial long before the review. The tutorial helped me a lot to enter an area of statecharts quickly. I'm still fsm/statecharts newbie though. Most things are quite understandable. However, code snipsets are quite complex. On my first reading, I stopped trying to understand them after a second of third sample. May be it's only me who reads complex stuff in a such way but the fact that simple_state class accepts up to five parameters gives a hint to a user that there shall be not_so_simple_state class that accepts 6+ or is more complex in other ways. This appears at the beginning and I suspect some will stop reading the tutorial right at that point. I'd suggest using some smart intendation at least.
* What is your evaluation of the design? What features are supported better by alternative FSMs? What could be added (or removed) from the library?
I don't like the design. Primary goal of the library is to be close to UML and the secondary goal is performance. UML conformance is pushed so strongly that any attempt to improve performance makes design worse. For example, deferral<XXX> requires that event is allocated using new and pointed by intrusive_ptr. Ordinary events can be allocated differently. If performance was out of library scope, the library could copy _all_ events into heap-allocated buffer. From user point of view, all event kinds looked the same, then. I believe that performance is not so important because even optimized, the library is too slow for FSM. According to docs, process_event function has 10 steps and there is a search in a couple of steps. This means loops and branching. I always thought that process_event in typical FSM is single jump followed by indirect function call. There are other things I don't like. One example is that transit call in react is similar to "delete this;" The "Unstable state" and "Unstable state machine" sections are too among things I don't like. They exist because transition is a complex process which involves multiple destructions and constructions. Not only it affects performance but it complicates recovering from exception.
* The library documentation contains few not yet solved issues (name, separating the library into two parts, exception handling). What is you opinion here?
I'm stongly against name FSM because the library doesn't give enough guarantees common in FSM world. Because the library is not predicitable in real-time terms and because FSM and RT systems often come together, this name can confuse some users. Boost.Statecharts would be much better, in my opinion.
* What is your evaluation of the implementation? Are there parts of code too obscure or duplicating exiting Boost functionality? Can something be factored out to standalone library or among general utilities?
I only searched for words virtual, typeid and throw. In the end I understood that code is as complex as I thought yet managable.
* Are there performance bottlenecks? Does the library design fit requirements of real-time systems? How is it useable for embedded systems? Is the documentation clear on performance tradeoffs?
I don't think it's acceptable for real-time systems.
* What is your evaluation of the potential usefulness of the library? Can you compare this FSM to other implementations?
It's definitely useful as a library for representing UML statechart but it covers only subset of FSM. I can compare it briefly with my experimental example which I use to verify a power of *overloads* library. It has very simple model: 1. Every state has a type that identifies this state 2. State machine stores current state by value => state-locale storage 3. Each transition is a function that returns new state by value, accepts current state in a first argument and processed event in a second argument. void return means no transition, only in-state reaction. 4. One state type can be derived from another and several transition functions that lead to one state from different states with common base can be replaced with one tranition function which lead to the same state from common base of those states. 5. All transitions are functions defined in state machine. 6. State machine change state always like this: template<class Event> void process_event(Event const& event) { // m_current_state is a member of state machine, // it behaves as variant<State1,State2,...> m_current_state = transition(m_current_state, event); } The model is simple, state changing is atomic, more complex functionality can be built on top either by user or by other layer of the framework. Extensibility is not yet solved but I'm sure the solution exists. Sample code: http://tinyurl.com/3swzq or http://cvs.sourceforge.net/viewcvs.py/cpp-experiment/overloads/libs/overloads/example/fsm.cpp?rev=1.4&view=auto
* Did you try to use the library? With what compiler? Did you have any problems? Do you have tips for better support of older compilers? What are possible portability problems?
Never had neither chance nor time.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Quick reading a couple of times and few hours of analysis.
* Are you knowledgeable about the problem domain?
Only common knowledge.
And finally, every reviewer should answer this question:
* Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
I'm afraid not. Either it should be repositioned as Boost.Statecharts or redesigned for better real-time support. -- Alexander Nasonov