boost-ext / sml

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

Vector of state machines looses state on resize #247

Closed maximilianriemensberger closed 5 years ago

maximilianriemensberger commented 5 years ago

When I hold a vector of state machines std::vector<sml::sm<states>> of size 1, process a transition, resize the the vector to size 32, then the first element of the vector gets reset back to the initial state. Basically the state machine silently looses its state. Below is a minimal example plus a lint to wandbox.

Minimal example (Wandbox https://wandbox.org/permlink/NHNUvaPqrj4RsYt5):

#include <cassert>
#include <vector>

namespace sml = boost::sml;

struct e1 {};
struct e2 {};

static constexpr auto s1 = sml::state<class s1>;

struct states {
    auto operator()() const noexcept {
    using namespace sml;
    const auto idle = state<class idle>;
    return make_transition_table(
       *idle + event<e1> = s1
      , s1 + event<e2> = X
    );
  }
};

int main() {
  std::vector<sml::sm<states>> sm;
  sm.resize(1);
  sm[0].process_event(e1{});
  assert(sm[0].is(s1));
  sm.resize(32);
  assert(sm[0].is(s1));  // <-- assertion triggers
}

Tested with clang 6.0.1 and gcc 8.2.0.

maximilianriemensberger commented 5 years ago

A raw memcpy fixes the problem (Wandbox https://wandbox.org/permlink/gErfE0xoJpxBqd8W):

int main() {
  std::vector<sml::sm<states>> sm;
  sm.resize(1);
  sm[0].process_event(e1{});
  assert(sm[0].is(s1));
  //sm.resize(32);
  //assert(sm[0].is(s1));

  decltype(sm) sm2;
  sm2.resize(32);
  std::memcpy(sm2.data(), sm.data(), sm.size() * sizeof(*sm.data()));
  assert(sm2[0].is(s1));  // <-- succeeds
}

But I 100% sure that's UB and gcc 8.2.0 even warns about that.

maximilianriemensberger commented 5 years ago

I also think that the state machine type should be trivial if it has no members. That would greatly speed up vector resizing as std::vector could use memcpy.

maximilianriemensberger commented 5 years ago

I think the error is in https://github.com/boost-experimental/sml/blob/a3d295ba826ecd17cae8ec587a2c110f9deef2ee/include/boost/sml/back/state_machine.hpp#L423

It should really be

         sm(sm &&other) : deps_{other.deps_}, sub_sms_{other.sub_sms_} {}
maximilianriemensberger commented 5 years ago

Suggested solution: Let the compiler figure out the move ...

  sm(sm &&other) = default;
  sm &operator=(sm &&other) = default;

... and then the empty sm even satisfies is_trivially_move_constructible.

maximilianriemensberger commented 5 years ago

248 fixed this issue