Hi Augustus [snip]

My main interest in a library like fsm lies in correctness and simplicity, not performance.

Which also happens to be the focus of this library.

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).

Have you ever had a look at other FSM libraries? I think boost::fsm is less messy than any other library I've seen.

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;

Actually there is, but I found out about this only recently (Peter Dimov mentioned this in an unrelated post) and haven't changed the docs because I feared that this does not work on all compilers: Instead of struct Greeting; struct Machine : fsm::state_machine< Machine, Greeting > {}; you can write: struct Machine : fsm::state_machine< Machine, struct Greeting > {};

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

You consider this more readable? Sorry, I have to disagree. For entry and exit actions and extended state variables I consider my approach superior, for reaction definition, your approach seems about equivalent.

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.

I don't understand, you do define states top-down in boost::fsm.

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

One sure can but that would duplicate mpl::list<> functionality to a certain degree.

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.

Given that the approach you outlined above really works, I think you can use lambda for transitions without changing a single line of boost::fsm, you simply define your own reaction that accepts typeofed lambda expressions instead of function pointers for transition actions. I'm not so sure whether the same is possible with entry/exit actions but I'm inclined to think that it is (that would require deriving your own state class template from simple_state).

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

Well, it's the other way round for me ;-). Most people get used to the syntax of boost::fsm quite quickly. Also, I've received feedback that people really like the fact that they can easily put the bodies of complex actions in cpp files, I *guess* that would be a little more difficult with your approach. [snip]

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

I don't think there is that much to learn: state_machine class template: 1. Param: The most-derived subclass 2. Param: Initial state simple_state, state class templates: 1. Param: The most-derived subclass 2. Param: Context 3. Param: Reactions 4. Param: Inner initial states 5. Param: History Reactions: 1. Param: Event 2. - 4. Params: transition destination & transition action (only present in transitions) event class template: 1. Param: The most-derived subclass I'd say it takes one day tops of playing around with boost::fsm to learn this. I did consider using named template parameters, but rejected it because it would add to the amount of text you have to write and would offer a benefit only for beginners.

tutorial just made my head spin, especially trying to figure out how states and machines get tied together (this is still unclear to me).

I guess you mean how the implementation ties them toghether? As a user, why do you need to know that? This might be part of a future implementation documentation but in general I think users can live very well without that knowledge.

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.

As I've hinted throughout the comments above, I think your approach is worse than mine for regular users, who IMHO are the most important part in the picture.

* 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.

As I already mentioned in another post: Targetting every C++ developer would require an *additional* document at least as thick as the current tutorial. I think it is reasonable to expect previous exposure to FSMs. I also do give links to FSM introductory material. That might not be enough but then again I don't think it is my duty as library writer to educate people about FSMs in general.

In general, I would drop all mention of UML except for some appendix somewhere (and the reference manual too).

Why?

"Getting Started" and "Target Audience" should be in a differenct document, and

Please make a suggestion.

"How to read this tutorial" should be removed.

Why?

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!

You're the second person to request that. I personally think it is better to not clutter code with too many comments. If people think it is better to have all comments in the code, I'll sure change it.

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.

Why?

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 ;)

You mean the mapping between the pressed key and the generated event? Why?

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,

It is not: http://tinyurl.com/5mjra (State-local storage)

but I would tend to assume that state-local-storage is maintained throughout unless I explicitly reset it.

Why?

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.

I don't know what accept states are. UML doesn't have them. [snip]

And lastly, thanks to all involved in bringing us boost::fsm.

You're welcome.

Despite my critique, this is first-rate work.

Thanks, and thanks for your review! Regards, -- Andreas Huber When replying by private email, please remove the words spam and trap from the address shown in the header.