I hesitate to call this a review; it's more my reactions looking through the fsm documentation and examples--I have not actually used it yet. I feel like these comments should have been given a long time ago, but I guess I missed all the pre-review discussion. I'm not an expert in this area, but I feel comfortable with the ideas and have fooled around with making my own state machines in the past. My main interest in a library like fsm lies in correctness and simplicity, not performance. Before I delve into answering the "official review questionare," I want to make some overall comments. First, it is obvious to me that this library has a lot of potential. The authors have thought through a lot of issues that I've never considered. My problems mainly lie with the syntax and documentation, and I think problems with the latter stem from the former. I'd like to see some discussion on the matter. Overall, I feel the syntax is too burdensome and the organization of code that I have to write is a hopeless mess (at least, looking at the examples). For simple cases, I really want to see a way of defining a fsm with less line noise and less indirection. I haven't thought through the ramifications, but it just seems to me that some mpl, lambda, and BOOST_TYPEOF loving should make something roughly akin to the following possible: { using namespace boost::fsm; using namespace boost::lambda; // I guess there's no way to avoid declaring // these ahead of time?? class Machine, State1, State2, Event1, Event2; struct S1Data { int i; S1Data() : i(0) {} }; typedef simple_state< Machine, S1Data, BOOST_TYPEOF( cout << constant("Entering State1.") ), BOOST_TYPEOF( cout << constant("Exiting State1.") ), on_event<Event1, BOOST_TYPEOF( _1->i++ ) >, transition<Event1, State2> > State1; // the "data" member of State1 has type S1Data, and is // passed into event handlers struct S2Data { int j; S2Data() : j(0) {} }; typedef simple_state< Machine, S2Data, BOOST_TYPEOF( cout << constant("Entering State2.") ), BOOST_TYPEOF( cout << constant("Exiting State2.") ) > on_event<Event2, BOOST_TYPEOF( _1->j++ )>, transition<Event2, State1> > State2; // the "data" member of State2 has type S2Data, and is // passed into event handlers // It seems that in the propsed fsm library, // state composition is specified bottom up, // and I'm not sure what the motivation is. // We're all used to writing classes top-down. typedef state_machine< State1, State2 > Machine; Machine m; m.initiate(); // process events while (whatever) m.process_event(blah); // I don't quite understand why state_cast is necessary // versus just overloading regular casting. cout << "State1 had Event1 called " << ((State1)m).data.i << " times." << endl; cout << "State2 had Event2 called " << ((State2)m).data.j << " times." << endl; } I'm sure I've messed up some details sine I don't really get why some of the fsm mechanisms are there. Also, I'm ignoring for the moment that member dereferencing is a little uglier with lambda, so _1->i++ would have to be (_1->* &S1Data::i)++. I assume there exists a way to make state-specific data available to a lambda expression in a clean way. I've also ignored sticking in lots of mpl::vector or mpl::lists, as it's my impression that with mpl one can iterate through the template arguments and figure out if they are events or transitions or whatever. So I smushed together the different elements, and presumed for < 10 arguments (or whatever, using PREPROCESSOR lib to configure that) the mpl::vector could be omitted. Finally, even though I've not really tested BOOST_TYPEOF, they use lambda expressions as an example, so I don't think it's entirely out of line of think that it might work. Now, I don't know if this example has really reduced the total number of characters required to be typed, but it is more clear to me. I realize that I've seperated the state data out into a seperate struct; personally, I like that. More importantly to me, I can look at those typedefs and reasonably guess what's being defined. If you made FSM_ENTER and FSM_EXIT macros to alias BOOST_TYPEOF you would make it clear what those entries were for. In general, you don't have to keep refering back and forth to the reference manual to figure out what each entry is. Looking at the examples in the tutorial just made my head spin, especially trying to figure out how states and machines get tied together (this is still unclear to me). This kind of "inline" code would be great for lots of cases where you just want to increment/decrement some counter or push/pop some variable. Maybe I'm hoping for too much magic, but I feel that at least little magic or secret sauce is needed here. * What is your evaluation of the documentation? How easy (or hard) it is to understand library features? What can be improved? All the other reviewers have said nice things about the documentation, but I had a lot of trouble with the tutorial. As already mentioned by others, totally targetted at the wrong people. In general, I would drop all mention of UML except for some appendix somewhere (and the reference manual too). "Getting Started" and "Target Audience" should be in a differenct document, and "How to read this tutorial" should be removed. Then, before you get into the C++ code for "Hello, World!", you should work through "Hello, World!" and maybe "StopWatch" using pseudo-code. Even if you can work out some magic like I hope for, I think you need to map out the basic process with pseudo-code, like this: // Hello World example // First, we have to declare all of our states // and events ahead of time. This is because // all states can transition to each other, // so you can't necessarily define them in order. class mystate1, mystate2, event1, event2, etc; // Next, we're going to define some states // for our machine. Each state definition // indicates what other states it transitions // to on each input, buried in "bunch_of_stuff" typedef simple_state< bunch_of_stuff > mystate; typedef simple_state< bunch_of_stuff > mystate2; // etc // Then, we're going define a machine with // states. The first listed state is the default typdef state_machine<mystate, mystate2> Machine; // etc, in this style In general, all of your "remarks" and "comments" need to be in the code! I think the example state machines are basically OK, but I think that it is absolutely essential that you include a simple example that is alphabet-based. In fact, some helper functions for alphabet-based languages are probably a good idea, just to make the tutorial easier. In particular, you should be able to create letters-as-events, and dispatch them easily. The switch statement for dispatch in the Camera example is scandalous ;) The reference manual seems ok, although I didn't really use it. The rationale tended to feel unconvincing, although I'm not well informed on the issues. * 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'm confused about why entering/exiting states causes construction / destruction. It's taken as a given, but I would tend to assume that state-local-storage is maintained throughout unless I explicitly reset it. I'm also surprised by the lack of "accept" states. Maybe it's not universal, but my theory certainly had them, and it's just disconcerting for them not to be there. * The library documentation contains few not yet solved issues (name, separating the library into two parts, exception handling). What is you opinion here? boost::fsm is good, IMO. Don't know about seperating libraries. * 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? Didn't look. * 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? No comment. * What is your evaluation of the potential usefulness of the library? Can you compare this FSM to other implementations? Tremendous potential. I can't compare directly to other FSM libraries/tools, but just like spirit, eliminating the need for extra tools is a great benefit. * 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? No. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Looked through docs and examples, thought carefully about what I wanted, and poked around to see what might be feasible. Many hours. * Are you knowledgeable about the problem domain? Not an expert, not completely ignorant. 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 think that boost::fsm will benefit greatly from some further consideration. The Serialization library took two tries and was much improved, to good effect. I can be accused of hoping for too much "magic" syntax, but it'll take a lot of convincing that the current way is best. I'm not unreasonable, but I haven't seen any discussion on it, and Spirit, Lambda, Serialization, and others given me high expectations for usability. And lastly, thanks to all involved in bringing us boost::fsm. Despite my critique, this is first-rate work. Thanks- Augustus __________________________________ Celebrate Yahoo!'s 10th Birthday! Yahoo! Netrospective: 100 Moments of the Web http://birthday.yahoo.com/netrospective/