0

I'm trying to implement State pattern in C++. Here an example code:

#include <cstdio>

class Context;

class State {
public:
    virtual ~State() = default;

    explicit State(Context *context) : context_(context) {}

    virtual void Print() = 0;

    virtual void ChangeState() = 0;

protected:
    Context *const context_;
};

class Context {
public:
    void SwitchState(State *state) {
        delete this->current_state_;
        this->current_state_ = state;
    }

    State *GetCurrentState() const {
        return this->current_state_;
    }

private:
    State *current_state_ = nullptr;
};

class FirstState : public State {
public:
    explicit FirstState(Context *context) : State(context) {}

    ~FirstState() override {
        printf("FirstState destructed. Address: %p\n", this);
    }

    void Print() override {
        printf("First state print..\n");
    }

    void ChangeState() override;
};

class SecondState : public State {
public:
    explicit SecondState(Context *context) : State(context) {}

    void Print() override {
        printf("Second state print..\n");
    }

    void ChangeState() override;
};

void FirstState::ChangeState() {
    printf("First state changed to second state\n");
    this->context_->SwitchState(new SecondState(this->context_));
    printf("First state changed to first state\n");
    this->context_->SwitchState(new FirstState(this->context_));
    printf("First state changed to second state\n");
    this->context_->SwitchState(new SecondState(this->context_));
}

void SecondState::ChangeState() {
    this->context_->SwitchState(new FirstState(this->context_));
    this->context_->SwitchState(new SecondState(this->context_));
    this->context_->SwitchState(new FirstState(this->context_));
}

int main() {
    Context context;

    context.SwitchState(new FirstState(&context));

    context.GetCurrentState()->Print();
    context.GetCurrentState()->ChangeState();
    context.GetCurrentState()->Print();
    context.GetCurrentState()->ChangeState();

    return 0;
}

I have a segmentation fault error because in this implementation after calling Context::SwitchState from the State, the current state is deleted and after returning from SwitchState method I can't use this, but sometimes I have to. I tried to use shared pointers, it helped a little, because I didn't get segmentation fault anymore, but for some reason destructor of the state is called anyway. I need to create new instances of states when switching it because I need to pass some arguments to the changed state. Also, I don't know how to solve this problem in general because all references to the state are lost after switching state, only remaining reference is this. I have some bad ideas that may solve that problem, but they are ugly, for example to make SwitchState just saving new state, and call some RealSwitchSavedState at the end of every method in state. I think it would solve the problem, because in this case a reference to the state remains until the end of the method, but the code will be ugly.

Hope for your help! Thanks in advance!

zenno2
  • 425
  • 2
  • 7
  • 1
    "_but for some reason destructor of the state is called anyway_" You invoke `delete this->current_state_` in `Context::SwitchState`. Which.. Deletes the state, and invokes its destructor. And, since, you, essentially, from within implementations of `State::ChangeState`, delete the state, that the method is called on - dereferencing `this` of that state is UB after the deletion. Making only the first `Context::SwitchState` call non-UB. The best bet, in my opinion, is to rethink the entire architecture of relationships. – Algirdas Preidžius Jul 10 '21 at 21:44
  • Save `this->context_` in a local variable at the top of `FirstState::ChangeState`, use that variable for subsequent `SwitchState` calls, so you don't need to touch `this`. Still a very fragile design, but it'll solve your immediate problem. – Igor Tandetnik Jul 10 '21 at 21:44
  • @AlgirdasPreidžius, yeah, I know why it happens, but I want to find the best solution with the same architecture, because in Java I can implement it easy, in C++ I face this problem. – zenno2 Jul 10 '21 at 22:33
  • @IgorTandetnik, thanks, but in a real code I need to access a full object state, but not only context. I now that I can save to a local variable data I need, but it is not a best solution – zenno2 Jul 10 '21 at 22:33
  • 1
    Forget your code for a minute. Can you explain in words what is supposed to happen? In particular, why is `SwitchState` logically consistent, not involving the destruction of `*this`? – JaMiT Jul 10 '21 at 22:33
  • @JaMiT, it is consistent only in that example to demonstrate the segmentation fault after accessing a deleted memory. It is supposed to be a simple implementation of the `State` pattern, but creating a new instance when changing a state and deleting an old one, so is a problem, because it already deleted the old one and continues to use it after returning from SwitchState – zenno2 Jul 10 '21 at 22:43
  • @zenno2 Stating "it is consistent" does not make it so. (Also, when formatted as `State`, the word refers to your `State` class. The name of the design pattern does not get formatted as code, as in "State design pattern".) – JaMiT Jul 10 '21 at 22:48
  • There are a lot of allocations here, could you not find a better solution using `std::variant`? – WBuck Jul 11 '21 at 15:00
  • See https://stackoverflow.com/q/133214/1168342 – Fuhrmanator Jul 14 '21 at 13:33

1 Answers1

1

You have a lot of allocations going on here. I'm curious if you could instead use an std::variant to achieve what you want.

Implementation

#include <variant>
#include <type_traits>
#include <iostream>

template<typename... Ts> struct Overload : Ts... { using Ts::operator( )...; };
template<typename... Ts> Overload( Ts... ) -> Overload<Ts...>;

class StateBase { 
public:
    virtual void print_state( ) const = 0;
    virtual ~StateBase( ) = default;
};

class FirstState : public StateBase {
public:
    void print_state( ) const override {
        std::cout << "First state\n";
    }
};

struct SecondState : public StateBase {
public:
    void print_state( ) const override { 
        std::cout << "Second state\n";
    }
};

struct ThirdState : public StateBase {
public:
    void print_state( ) const override { 
        std::cout << "Third state\n";
    }
};

template<typename ...States>
class Fsm { 
    static_assert( ( std::is_base_of_v<StateBase, States> && ... ), "State must derive from StateBase" );

public:
    template<typename T, 
             typename En = std::enable_if_t<std::is_base_of_v<StateBase, T>>>
    explicit Fsm( T initial_state ) 
        : state_{ std::move( initial_state ) }
    { }

    template<typename T, 
             typename En = std::enable_if_t<std::is_base_of_v<StateBase, T>>>
    void change_state( T state ) {
        state_ = std::move( state );
    }

    template<typename T, 
         typename En = std::enable_if_t<std::is_base_of_v<StateBase, T>>,
         typename ...Args>
    void change_state( Args&&... args ) {
        state_.template emplace<T>( std::forward<Args>( args )... );
    }

    void print_state( ) {
        std::visit( Overload { [ ]( const StateBase& s ) { 
            s.print_state( ); } }, state_ );
    }
private:
    std::variant<States...> state_;
};

Usage

int main( ) {
    Fsm<FirstState, SecondState, ThirdState> fsm{ ThirdState{ } };
    // Prints Third State.
    fsm.print_state( );

    fsm.change_state( SecondState{ } );

    // Prints Second State.
    fsm.print_state( );

    fsm.change_state<FirstState>( );
    
    // Prints First State.
    fsm.print_state( );
}

Now I don't know exactly what your use-case is, or if this meets your requirements but you may be able to change this example to achieve your desired functionality.

WBuck
  • 5,162
  • 2
  • 25
  • 36