Is this C++ code safe?


  • Banned

    Question to standard lawyers. Kinda long snippet because C++ is overly verbose with class declarations:

    class Manager;
    
    class State
    {
    public:
    	State(Manager& manager): manager(manager) {}
    	virtual ~State(){}
    
    	virtual void doSomething() = 0;
    	
    protected:
    	Manager& manager;
    };
    
    class Manager
    {
    public:
    	void setState(std::unique_ptr<State> new_state)
    	{
    		state = std::move(new_state);
    	}
    	
    private:
    	std::unique_ptr<State> state;
    };
    
    class StateA: public State
    {
    public:
    	StateA(Manager& manager): State(manager) {}
    	
    	void doSomething();
    };
    
    class StateB: public State
    {
    public:
    	StateB(Manager& manager): State(manager) {}
    
    	void doSomething();
    };
    
    void StateA::doSomething()
    {
    	manager.setState(std::make_unique<StateB>(manager));
    }
    
    void StateB::doSomething()
    {
    	manager.setState(std::make_unique<StateA>(manager));
    }
    

    If Manager called doSomething() on its state, the state would replace itself with a new state - and by the magic of unique_ptr, the old state would be destroyed. The destruction would happen before the old state returns from doSomething(), though - but at the very end, when the state's members aren't in use anymore. Is this legal, well-formed, safe C++? Are there any nasal demons ahead?



  • If I understand correctly, this basically boils down to a delete this from a virtual function. Should be safe - see e.g., the c++ faq.

    Also, IIRC, this is a fairly common thing when using something like IUnknown. MSDN has examples that do delete this in this context.


  • Winner of the 2016 Presidential Election

    I agree with everything @cvi just said.



  • I'd feel better about it if the class replacement was done from the caller, instead of inside the class itself. It feels wrong the way it is.

    myStateB = myStateA.doSomething();


  • Banned

    @blakeyrat said:

    I'd feel better about it if the class replacement was done from the caller, instead of inside the class itself. It feels wrong the way it is.

    Except the caller doesn't know what the new state should be. Or there might be no state change. It's all decided by the state object. And I'm not keen on putting too many layers of indirection in my code.

    @cvi, thanks for help.



  • @Gaska said:

    Except the caller doesn't know what the new state should be.

    Right; but you can still do:

    State myState;
    myState = myStateA.doSomething(); // returns either StateA or StateB
    

    ... right? My C++ is rusty as hell.

    In any case, I'm not saying it's wrong, I'm just saying it rubs me the wrong way.


  • Banned

    I think what you mean is the following:

    myState = myState.doSomething();
    

    Which would work too, I think.



  • Yeah exactly. That would make what's happening more explicit, less magic. I just don't remember the inheritance rules in C++ at all.


  • Winner of the 2016 Presidential Election

    I agree with blakey. Something like:

    manager.setState(oldState.getNextState());
    

    looks a lot better than the magic above. Unless there's a specific reason I don't know of, it doesn't make sense for the State to have a reference to the Manager object.


  • Banned

    @blakeyrat said:

    Yeah exactly. That would make what's happening more explicit, less magic. I just don't remember the inheritance rules in C++ at all.

    Actually, my correction had nothing with C++, but with coding in general.

    @asdf said:

    Unless there's a specific reason I don't know of, it doesn't make sense for the State to have a reference to the Manager object.

    It was my attempt to implement state machine pattern - ie. the manager's behavior changes due to something happening. For me, it makes more sense for state transition to happen automagically than explicitly - but I might be wrong. Also, state will make heavy use of manager's stuff, so the reference will be there anyway.


  • Winner of the 2016 Presidential Election

    @Gaska said:

    Also, state will make heavy use of manager's stuff, so the reference will be there anyway.

    Hm, ok, in that case it might make sense to hide the state inside the Manager and let the rest of the code interact with that.

    void Manager::doSomething()
    {
        state = state.doSomething();
    }
    

    I'd really try to avoid the above code, if I were you. It looks unintuitive and somehow wrong to me.


  • Banned

    @asdf said:

    I'd really try to avoid the above code, if I were you. It looks unintuitive and somehow wrong to me.

    Assuming you mean the snippet you've posted, that's exactly what I'm trying to avoid by this assignment magic.


  • Winner of the 2016 Presidential Election

    @Gaska said:

    Assuming you mean the snippet you've posted

    That's exactly the opposite of what I meant, but since you obviously don't like my suggestion (the snippet), I'll shut up now. ;)



  • @Gaska said:

    myState = myState.doSomething();

    This forces you to always return a new state, or intruduce some extra logic (e.g., returning a null pointer) to remain in the current state.

    IMO, if the states are internal and hidden away, and everybody only ever works through the Manager object, then the orginal approach is fine. Just make sure that nobody except the manager ever gets hold of references to State instances.



  • Alternatively, you could create a setNextState method which does not replace the current state, but stores the next state in a second pointer. After doSomething returns, you can check if nextState!=NULL and replace the current state.

    Or you could use singletons for the various states (think enum, but with methods). Then you can use plain pointers and don't have to worry about destroying anything. The manager can be passed as a parameter if necessary. That won't work if your state objects need to store additional, uh, state.

    Or you could store the state as a pointer to a member function of the manager class (i.e. like a virtual method call, but without all those classes). That won't work if you need more than one method, though.


  • Java Dev

    Ugh, state machines. Now I'm from a C background, and this might be slightly different in an OO environment, but I'm going to give my opinion anyway...

    Consider whether you really need them. In my opinion, if you can write plain old function-based code, it will probably be significantly cleaner than the state machine alternative.

    That said I have run into some cases where state machines can be useful:

    • If the input to your state machine is not readily available. Keeping the current processing state may be more efficient than doing large amounts of input buffering, to a sufficient degree that it excuses the less readable code.
    • If the logic is not available at compile-time, for example because you need to apply configurable patterns to an input.

  • Banned

    @asdf said:

    That's exactly the opposite of what I meant, but since you obviously don't like my suggestion (the snippet), I'll shut up now.

    Well, your snippet basically means "for every method in State, make a stub method in Manager". Makes sense in some simple cases, but not this one. Besides, it's completely unrelated to my problem.

    @cvi said:

    This forces you to always return a new state, or intruduce some extra logic (e.g., returning a null pointer) to remain in the current state.

    And that's why I don't like @blakeyrat's solution that much.

    @fatbull said:

    Alternatively, you could create a setNextState method which does not replace the current state, but stores the next state in a second pointer. After doSomething returns, you can check if nextState!=NULL and replace the current state.

    This seems hacky and unnecessary, since the original solution works.

    @fatbull said:

    singletons

    Burn in hell.

    @fatbull said:

    Or you could store the state as a pointer to a member function of the manager class (i.e. like a virtual method call, but without all those classes). That won't work if you need more than one method, though.

    And I need more than one method.

    @PleegWat said:

    Consider whether you really need them.

    Yes, I absolutely, totally, undoubtedly need them. I'm preparing to refactor a really nasty piece of code with dozens of nested ifs, unclear execution paths and terrible data flow. State machine is the best way to solve "when something happens, turn the behavior upside down" problem.



  • @Gaska said:

    @fatbull said:
    singletons

    Burn in hell.

    That's the spirit! :D
    Filed under: seriously, singletons have never been good advice


  • Java Dev

    @Gaska said:

    unclear execution paths

    Not sure if state machines are going to resolve that bit... course if you're effectively working with state already anyway codifying stuff in a state machine may improve matters. I've got some code like that I'm not allowed to touch.


  • Banned

    @PleegWat said:

    Not sure if state machines are going to resolve that bit...

    Yes they are. No more jumping between random functions, no more four levels of if nesting, no more guessing which booleans are set to true and which optionals have values in a given scenario.


  • Winner of the 2016 Presidential Election

    @cvi said:

    IMO, if the states are internal and hidden away, and everybody only ever works through the Manager object, then the orginal approach is fine. Just make sure that nobody except the manager ever gets hold of references to State instances.

    That's pretty much what I was trying to say. All of that logic should be encapsulated in the Manager object somehow.

    But then I belgiumed up by writing a code snippet and subsequently referring to the "above code" when I actually meant the original code snippet posted by Gaska.



  • I don't know C++, but from the sound of it, you are implementing a state monad, and are using the manager object to hide the plumbing. That's fine, but consider whether a first-class state monad would be clearer. http://bartoszmilewski.com/2014/02/26/c17-i-see-a-monad-in-your-future/

    (Maybe, maybe not. I'm guessing monads in C++ won't be particularly idiomatic for a few more years)



  • @Captain said:

    I'm guessing monads in C++ won't be particularly idiomatic for a few more years

    C++17 is going to be the one that subsumes every existing paradigm in the whole of software engineering, isn't it?


  • Banned

    If I understand correctly, this monad thing isn't going to help me at all. There's some incoming events that change state of the system (not to be confused with Manager's State), and Manager needs to react to it by sending more events. The state machine is there just to reduce the amount of ifology needed. I don't see how monads can help here.



  • Yes, it's pretty much just a different notation for what you're already doing. Never mind. 😄



  • Now I think about it, I'm not sure I've ever seen a C++ state machine implementation which I was 100% happy with. They always seem to end up a bit boiler-platey...


  • Banned

    I'm not sure I've ever seen any C++ that wasn't boiler-platey. Even making foreach-compatible lazy-evaluated iterator requires tons of boilerplate (which is no-brainer in other languages).



  • Well, the idea was to preallocate all state objects and use a pointer to choose the active one. Non-static member variables are probably a better choice.


    Filed under: 🔥🔥🔥🔥🔥



  • @Gaska said:

    I'm not sure I've ever seen any C++ that wasn't boiler-platey.

    True, but the preprocessor can do a lot of of your boilerplating for you...
    Evil ideas: ⬅ 🔄 ⬆ 🔄 ↘



  • @Gaska said:

    Burn in hell.

    @tar said:

    Filed under: seriously, singletons have never been good advice

    Let's say that you have a fireball that makes a boom sound when it hits an object.

    Without a singleton:

    fireball->getOwnedScene()->getSoundManager()->playSound(boomSound, fireball.getPosition());

    With a singleton:

    SoundManager::playSound(boomSound, fireball.getPosition());

    Can you come up with a non-singleton implementation that doesn't require storing a parent in every class and/or walking up the hierarchy anytime you want to do something non-trivial?



  • @Groaner said:

    Can you come up with a non-singleton implementation that doesn't require storing a parent in every class and/or walking up the hierarchy anytime you want to do something non-trivial?

    Handwaving, but I'd probably pass the soundManager into the function on fireball which wants to play the sound...

    void FireBall::playBoomSound(SoundManager *sm, BoomSound *boomSound) {
        sm->playSound(boomSound, getPosition());
    }
    

    You can argue it's tedious to pass objects around this way, it probably is tedious this way, but it makes it a lot easier to split your codebase over several cores if each subsystem of the code explicitly receives the references to only the objects it needs.



  • @tar said:

    Handwaving, but I'd probably pass the soundManager into the function on fireball which wants to play the sound...

    In practice, it's going to happen in some generic event handler like this, which I could see quickly gaining parameters (or a massive HitEventArgs class that's going to encapsulate the SoundManager among other things):

    void Fireball::onHit(Object &otherObject)
    {
         // deal damage
         dealDamage(otherObject);
         
         // make a sound
         ...
    
         // mark self collided
         collided = true;
    }
    

    Granted, you might be able to get around this by extracting the behavior and making it happen when iterating through all collisions...

    void Scene::doCollisions()
    {
      for(CollisionIterator ci = collisions.begin();  ci != collisions.end(); ci++)
      {
          ci->firstObject.dealDamage(ci->secondObject);
          soundManager->playSound(ci->firstObject.getHitSound(), ci->firstObject.getPosition());      
          ...
      }
    }
    

    Although in this case, it becomes difficult to have polymorphic behavior for different types of colliders, unless it can fit the existing pattern.



  • @Groaner said:

    In practice, it's going to happen in some generic event handler like this, which I could see quickly gaining parameters (or a massive HitEventArgs class that's going to encapsulate the SoundManager among other things):

    Not necessarily. We could go fully-event driven at this point, and completely decouple everything:

    Fireball::Fireball() {
        register_event_handler<HitEvent>(&Fireball::on_hit);
        //...
    }
    
    void Fireball::on_hit(const HitEvent &evt) {
        post_event( DamageEvent(evt.otherObject, ...) );
        post_event( SoundEvent(this->position, this->boomSound, ...) );
        post_event( ParticleEvent(this->position, this->colour, ...) );
        // do other hit-related stuff...
        Base::on_hit(evt); //possibly do this as well...
    }
    

    Now SoundManager is just an unrelated thing listening for its own events:

    SoundManager::SoundManager() {
        register_event_handler<SoundEvent>(&SoundManager::on_sfx);
    }
    
    void SoundManager::on_sfx(const SoundEvent &evt) {
        HW->PlaySound(evt.position, evt.sfx, evt.volume, ...);
    }
    

    I have (quite conveniently) glossed over event routing completely, but there's lots of different ways we can play this out at this point.


  • Java Dev

    Now it's just the sound manager's input queue that is a global static, instead of the sound manager itself. I don't really think that changes the amount of magic (no pun intended) going on.


  • Banned

    With Entity-Component System, you could have a fireball object have Sound component and Collision component. The Collision component upon hit would set Sound component to active state, and then, independently, the sound subsystem would check all objects that have Sound component if they were activated, and will play the appropriate sounds.



  • @PleegWat said:

    Now it's just the sound manager's input queue that is a global static, instead of the sound manager itself. I don't really think that changes the amount of magic (no pun intended) going on.

    Not really, I could have 15 different SoundManagers now, if I wanted. And copying events should hardly qualify as "magic"...


Log in to reply