search

boost-ext / sml

C++14 State Machine library
https://boost-ext.github.io/sml
Boost Software License 1.0
1.16k stars 179 forks source link

SML version change caused static assert fail in state machine constructor #617

Closed bjorn-jaes closed 7 months ago

bjorn-jaes commented 7 months ago

Expected Behavior

As part of a reinstall I changed from version 1.1.9 of SML to the latest version 1.1.11 and expected my code to build without any issues, as it had done before on versions less than 1.1.10.

Actual Behavior

When building the tests for my Jay library the tests fail to build with the following error message:

In file included from /workspaces/jay/tests/../include/jay/address_claimer.hpp:19, from /workspaces/jay/tests/../include/jay/address_manager.hpp:25, from /workspaces/jay/tests/address_manager_test.cpp:12: /usr/local/include/boost/sml.hpp: In instantiation of 'constexpr boost::ext::sml::v1_1_10::aux::missing_ctor_parameter::operator TMissing&() const [with TMissing = jay::address_claimer::st_claiming; typename boost::ext::sml::v1_1_10::aux::enable_if<(! boost::ext::sml::v1_1_10::aux::integral_constant<bool, __is_base_of(boost::ext::sml::v1_1_10::aux::pool_type_base, U)>::value), int>::type = 0; T = jay::address_claimer::st_claiming]': /usr/local/include/boost/sml.hpp:451:108: required from 'constexpr boost::ext::sml::v1_1_10::aux::pool::pool(boost::ext::sml::v1_1_10::aux::init, const boost::ext::sml::v1_1_10::aux::pool<TArgs ...>&) [with TArgs = {jay::address_claimer&, jay::network&}; Ts = {jay::address_claimer&, const jay::network&, jay::address_claimer::st_claiming&, jay::address_claimer::st_has_address&}]' /usr/local/include/boost/sml.hpp:1825:132: required from 'constexpr boost::ext::sml::v1_1_10::back::sm< >::sm(TDeps&& ...) [with TDeps = {jay::address_claimer&, jay::network&}; typename boost::ext::sml::v1_1_10::aux::enable_if<((sizeof... (TDeps) > 1) && boost::ext::sml::v1_1_10::aux::is_unique<boost::ext::sml::v1_1_10::aux::type<>, TDeps ...>::value), int>::type = 0; TSM = boost::ext::sml::v1_1_10::back::sm_policy<jay::address_claimer, boost::ext::sml::v1_1_10::back::policies::dont_instantiate_statemachine_class>]' /workspaces/jay/tests/../include/jay/address_manager.hpp:65:67: required from here /usr/local/include/boost/sml.hpp:394:53: error: static assertion failed: State Machine is missing a constructor parameter! Check out the missing_ctor_parameter error to see the missing type. 394 | static_assert(missing_ctor_parameter::value, | ^~~~~

The constructor with the missing parameter is statemachine in the code bellow:

address_manager(boost::asio::io_context &context, jay::name name, jay::network &network) : 
context_(context), network_(network), addr_claimer_(name), 
state_machine_(addr_claimer_, network), timeout_timer_(context)

With the state machine being declared as follows:

  jay::address_claimer addr_claimer_;
  boost::sml::sm<jay::address_claimer> state_machine_;

From I can interpret from the error message it seems to suggest that the i am missing a st_claiming instance from my constructor? Though I am not sure that is correct, as my state machine should only require an instance of an address_claimer and the only additional dependency should be a network instance.

  struct st_claiming{
    std::uint8_t address{J1939_NO_ADDR};
  };

My Suspicions

I am a bit stuck on interpreting the error message as am not familiar enough with template coding to interpret the SML source code. I did look thought the change log to see if I could spot anything obvious and was wondering if any of the following additions might be the cause of the issue:

This I imagine would be the most likely thing i could imagine to change the construction works. Thought I cant find any example or documentation on how anything would be that much different from before.

Steps to Reproduce the Problem

  1. Install Boost, Canary and SML (version 1.1.10+)
  2. Download Jay Library
  3. Try and build Jay Library

Specifications

kris-jusiak commented 7 months ago

Yeah, it's very likely to be related to the new policy. Looking at the diff https://github.com/boost-ext/sml/compare/v1.1.9...v1.1.10 that's pretty much all the changes in that area.

To get the old behavior, can you try with -DBOOST_SML_CREATE_DEFAULT_CONSTRUCTIBLE_DEPS?

Also, would it be possible to get godbolt link with reproduction, that would defo speed up the investigation. Thanks.

bjorn-jaes commented 7 months ago

I created a compiler explorer project (godbolt) here. I was able to reproduce the error there using a more minimal example from my library. I included the headers for SML version 9 and 11 in the project so that you can swap between them using the include at the start of the example.cpp file. I have not used compiler explorer that much before so didn't know if there was an easier way of including the library. In any case the code will compile on version 9 and fail on version 11.

kris-jusiak commented 7 months ago

Thanks, that really helpful. So indeed the default behavior has changed since version 10+ to improve the required storage requirements. It should be more visible in the error message, will work on that. Unfortunately, that's a breaking change. too. There are 2 ways to fix it without changing sml.

  1. Define BOOST_SML_CREATE_DEFAULT_CONSTRUCTIBLE_DEPS which brings old behavior

  2. Create missing dependencies and pass them via constructor

    address_claimer::st_has_address st_has_address{};
address_claimer::st_claiming st_claiming{};
boost::sml::sm<address_claimer> state_machine_{addr_claimer_, net, st_has_address, st_claiming};
    • https://godbolt.org/z/Kee1srbae (that should also take less storage for the state machine dependencies as there was a value and ref stored for default constructible objects)

Let me know if that works for you and if there is anything else which can be done to improve it?

bjorn-jaes commented 7 months ago

Solution

Thank you, I am pretty sure I now understand the issue. So my misunderstanding was that I thought that states did not count as dependencies. While in reality any object that is in guard or function / lambda (that is not the event) is a dependency. Given that all my states were default construable this was not that obvious. I will be fixing my code by passing in my states as dependencies (2).

Documentation

I checked the examples for data and see that it is still are using

define BOOST_SML_CREATE_DEFAULT_CONSTRUCTIBLE_DEPS

to allow the Interrupted object to be default constructed, so it does not need to be included in the state machine constructor. I also noticed that is example that an instance Connected is created directly in the constructor to change the default value of the state. This was also no allowed in the latest version. Not sure if that documentation in data example should be updated?