0
\$\begingroup\$

I've got the following classes: BaseWidget, ActiveWidget, and DetachedWidget. Both derived classes use resources provided by BaseWidget: vector and int. The current so-called "state" (represented by each derived class) is stored in Context as a unique ptr to base class.

Imagine that Context is like an entry point for the system. For simplicity Context's handle method does not take any parameter but in a real scenario, it would take some data.

Anyways, when one calls Contex's handle method, then the call is forwarded to the current "state" and then it processes the "command". As you can see, e.g. when the current state is represented by DetachedWidget, and when the handle method gets called, then DetachedWidget exchanges the current state to ActiveWidget passing to ActiveWidget's constructor one parameter which is an rvalue reference to BaseClass created by invoking std::move(*this). The purpose of this is that I want to pass all resources that DetachedWidget owns to the new instance. Of course, I could have passed e.g. vector, and int as separated parameters, but I knew that in every case I would like to exchange all resources, so taking the base class as a parameter and then passing it to move ctor of base class sounds like a quite good idea.

But I'm not 100% sure if there are any pitfalls that I'm not aware of yet. There should be no slicing issues nor dangling references and so on but... I'm especially interested in two aspects:

  • A derived class that has a constructor which takes rvalue reference to BaseClass
  • std::move(*this)
#include <memory>
#include <iostream>
#include <algorithm>
#include <random>
#include <vector>
#include <cassert>

class Context;
struct BaseWidget
{
  int x;
  std::vector<int> vec;

  virtual void handle(Context&) = 0;
};


struct Context
{
  std::unique_ptr<BaseWidget> widget;

  void set_widget(std::unique_ptr<BaseWidget> w)
  {
    widget = std::move(w);
  }

  void handle()
  {
    widget->handle(*this);
  }
};

struct ActiveWidget : BaseWidget
{
  explicit ActiveWidget(BaseWidget&& base)
    : BaseWidget(std::move(base))
  {
    assert(vec.size() == 2);
  }

  void handle(Context& ctx) override
  {

  }
};

struct DetachedWidget : BaseWidget 
{
  DetachedWidget()
  {
    vec.resize(2);
  }
  void handle(Context& ctx) override
  {
    ctx.set_widget(std::make_unique<ActiveWidget>(std::move(*this)));
  }
};


int main() 
{
  auto ctx = Context{};
  auto initial = std::make_unique<DetachedWidget>();
  ctx.set_widget(std::move(initial));
  ctx.handle();



    return 0;                                        
}

```
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Never let an object delete itself

I've gone down a similar road myself, and there are pitfalls everywhere. Just do not do it. The problem is that while there is no explicit delete, the fact that DetachedWidget::handle() calls ctx.set_widget() while ctx is the owner of the DetachedWidget object means that in effect, DetachedWidget is deleting itself. Before set_widget() even returns, that means the DetachedWidget object is no longer in a valid state. Any further execution of its member functions is undefined behavior.

There might be ways around this. You could make a widget's handle() return a function object which Context::handle() will then call the moment it gets it, and that function object can then call set_widget(). For example:

struct Context {
    ...
    void handle() {
        widget->handle(*this)(); // Note the extra parentheses
    }
    ...
};

struct DetachedWidget : BaseWidget {
    ...
    std::function<void()> handle(Context& ctx) override {
        return [this](Context& ctx){
             ctx.set_widget(std::make_unique<ActiveWidget>(std::move(*this)));
        };
    }
    ...
};

But this looks very contrived. Which brings me to:

Polymorphism might be the wrong solution here

Do you need to have a separate ActiveWidget and DetachedWidget at all? If they are already so intertwined, why not just have one Widget class that has a bool active member variable?

If either one of those widgets has a lot of state that the other doesn't need, and you don't want to pay for the memory usage, then perhaps just store that state in a separate object held in a std::unique_ptr member variable in Widget.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for this comment. As for deleting this, I will update my code, but as for the rest - believe me - this piece of code I presented above is just an example. The real solution is much more complicated. "States" have to react in a different way on a different commands. The first solution was using enums, flags and so on and it was a complete mess, plenty of nested if statements and so on. Once I created a solution I presented above, code magically became clear, easy to read and what's important - it started working correcly, just beacuse each "state" does only what its supposed to do. \$\endgroup\$
    – bielu000
    Dec 14, 2021 at 15:16
  • \$\begingroup\$ @bielu000 It would be much better then if you post the real code for review here, as we can then much better see what the actual issues are. It is fine to post longer pieces of code on Code Review. \$\endgroup\$
    – G. Sliepen
    Dec 14, 2021 at 16:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.