Re: [boost] Review request: Boost.FSM

From: Andrey Semashev (andysem_at_[hidden])
Date: 2007-01-03 11:27:25

Hello Steven,

Wednesday, January 3, 2007, 12:23:04 AM, you wrote:

> AMDG

[snip the announcement]

> 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?

It's a synonym for "most" here. These classes don't inherit from any
other classes.

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

I'll fix these three notes shortly.

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

Fixed this. Although I'm not quite sure if we have to try to evade all
compiler complains at "panic" warning levels. Trying to compile the
test for the library with /W4 I got huge amount of warnings like this
that originate from the test or other Boost components (lexical_cast,
for example).

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

Fixed.

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

I'd rather say, your scenario is possible here but not necessarily it
will happen. A compiler _may_ be smart enough to reorder expressions
if it may prove that it won't break the code behavior (which is true
here) and it will gain something (performance, for example) from it.
And even if it does so consider that Thread A in your example may be
interleaved by Thread B right between it checks "m_fInitialized" and
stores "true" in it. So it is actually possible that more than one
thread will execute the "true" branch of the "if" statement
concurrently. But we're on the safe side here because only simple
operations on POD types are involved in it.

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

In fact, it's the first file in the inclusion sequence that needs it.
I'd rather not to extract this only type to a separate file. I see two
ways to do about it:
- To add a note in the docs about where to look for this type. Though
a user should not care of the nature of this type.
- To move it to "prologue.hpp" in "details" directory. May be this
file would be better, what's your opinion?

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

Fixed. Though this was quite a surprise for me since I thought that
static functions have internal linkage and therefore may be defined in
headers.

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

Fixed.

-- 
Best regards,
 Andrey                            mailto:andysem_at_[hidden]
PS: Thanks for your feedback.