Re: [boost] Review request: Boost.FSM

From: Steven Watanabe (steven_at_[hidden])
Date: 2007-01-02 16:23:04

AMDG

Andrey Semashev wrote:
> Hi,
>
> I would like to ask for a formal review of the Boost.FSM library
> proposed here not long ago. I've uploaded the implementation, test and
> documentation into Boost.Vault, it is accessible with this link:
>
> http://tinyurl.com/yjozfn
>
> There were several changes in the library since the proposal:
> - Integral constants tagged events support added
> - Transitions maps support improved. Each transition may now decide in
> run-time to which state to transit. The transition map itself may be
> broken into several pieces - the common part and state-specific parts.
> - Added an implementation of thread-locking state machine. It is quite
> equivalent to the "state_machine" class but adds a tiny
> synchronization layer between environment and the machine.
> - The library has been split into several headers and put into its own
> directory.
> - The documentation have been improved.
>
>   
I just looked over state_machine.hpp and exceptions.hpp.

state_machine.hpp line 61
  //! This class is the very base for every state
state_machine.hpp line 88
  //! The very base class of a complete state machine. ...
What does very mean here?

state_machine.hpp line 89/96
Shouldn't StatesCountV and states_count have the same type?

the BOOST_STATIC_CONSTANTs do not have a namespace scope definitions (9.4.2)

Names containing double underscores are reserved (17.4.3.1.2).

state_machine.hpp line 296
      if (next_state_id != state_id)
Under msvc /W4 this causes warning C4127 conditional expression is constant

state_machine.hpp line 304/329
  ...((root_type*)this) ...;
static_cast<root_type*>(this)?

state_machine.hpp line 643
      void init(const states_compound_type* pStates)
      {
        // Race condition is possible here, but it is not significant
        // since only POD types are involved and the result of 
initialization
        // does not depend on number of threads or number of 
initializations.
        if (!m_fInitialized)
        {
          // Fill m_StatesInfo array for all states
          const root_type* pRoot = pStates;
          pStates->__init_states_info(reinterpret_cast< const char* 
 >(pRoot), m_StatesInfo);
          m_fInitialized = true;
        }
      }
This is just wrong. It is perfectly legal
for the compiler to reorder it to be
        if (!m_fInitialized)
        {
          m_fInitialized = true;
          // Fill m_StatesInfo array for all states
          const root_type* pRoot = pStates;
          pStates->__init_states_info(reinterpret_cast< const char* 
 >(pRoot), m_StatesInfo);
        }
then
Thread A
   if (!m_fInitialized)
   {
     m_fInitialized = true;
Thread B
   if (!m_fInitialized)
   {
   }
   //use m_StatesInfo.
   //die.

Exceptions.hpp

exceptions.hpp line 48
typedef unsigned int state_id_t;
Does this really belong in exceptions.hpp?
It took me a little while to find the definition.

exceptions.hpp line 67
  static std::string construct_type_name(std::type_info const& info)
This must NOT be static as it will result in ODR violations.
to define it in the header without making it inline write
  template<class T>
  std::string construct_type_name_impl(T const&) {
    //implementation here
  }
  inline std::string construct_type_name(std::type_info const& info) {
    construct_type_name_impl(info);
  }

c_str is used in what and is not protected by try.
Although this almost certainly does not throw
the standard does not forbid it to do so.

In Christ,
Steven Watanabe