Re: [boost] Review request: Boost.FSM
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:
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.
(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); }
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* 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
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.
(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); }
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* 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@mail.ru PS: Thanks for your feedback.
participants (2)
-
Andrey Semashev -
Steven Watanabe