3
\$\begingroup\$

I have a small class of 2D drawing utilities, Draw2D which is the global entry point for anything related to drawing. Currently, the class looks like this:

struct Draw2D
{
    std::shared_ptr<D2Label> CreateLabel (...);
    std::shared_ptr<D2Plot> CreatePlot (...);

    void Draw (D2Element& e);

    std::vector<std::weak_ptr<D2Element>> elements_;
};

struct D2Element
{ ... };

struct D2Label : D2Element
{ ... };

So far so good. The key point here is that Draw2D needs to know all it's elements so it can update them and for drawing, each element has to call back to its parent Draw2D instance. I.e. you cannot move elements between instances as all elements share resources provided by the parent. The solution I have right now is weak_ptr which get checked on each update of the Draw2D instance and if stale, the object is removed (meaning the user has deleted it in the meantime.)

The problem is that after the Draw2D instance died, of course all still existing instances are dead, too. Even though they are technically valid objects, you cannot use them safely any more. Even queries could fail if they touch the shared resources, and keeping the shared resource alive via shared_ptr does not help either because that just postpones the problems until the drawing backend dies (in which case all resources are invalidated.)

Is there some way to rewrite this code to make it clearer to the clients? I tried something like this, as I definitely want clients to be able to release resources early (i.e. before Draw2D dies.)

struct D2Element
{
    ~D2Element ()
    {
        deleter_ (this);
    }

    std::function<void OnDelete (D2Element*)> deleter_;
}

struct Draw2D
{
    void Release (D2Element* element)
    {
        // Remove from list of registered elements
    }

    ~Draw2D ()
    {
        // delete all remaining children
    }

    D2Label* CreateLabel (...)
    {
        auto label = ...;
        label->deleter_ = std::bind (&Draw2D::Release, this, _1);
        return label;
    }
}

This is ok but now the clients have to wrap it into smart pointers on their own by default.

\$\endgroup\$
  • \$\begingroup\$ I'm not sure if I understand the problem, but can't you overload Draw2D::Release(std::shared_ptr<D2Element> element) { Release(element.get();) } (with a cast if necessary) and then create auto label as a shared_ptr? \$\endgroup\$ – Felix Dombek Sep 12 '11 at 6:09
  • \$\begingroup\$ Ok I have worked through your code several times now and it is becoming clear that you have not disclosed a key piece of information. What is this shared resource, the drawing back end as you call it? Why is there a binding needed at all between the D2Element derived object types and the engine (Draw2D) that draws them? \$\endgroup\$ – dex black Sep 13 '11 at 12:19
1
\$\begingroup\$

Why not separate the 2D element into two parts.

  • The interface that can be used
  • The shared resource
    • Used by the Draw2D interface
    • Used by the D2Elements.

Then D2Elements does not need to depend on an object that can go out of scope, they have partial ownership of the shared resource (with any common data that they and the Draw2D needs.

class Draw2D
{
    // interface as before
    // Weak pointers as before.

    std::shared_ptr<SharedResource>   data;
};

class D2Element
{
    //... As before.

    std::shared_ptr<SharedResource>   sharedData;
};

Even of the Draw2D's lifetime ends, then the shared resource (because it is shared with any created children) will not die and thus not be destroyed.

\$\endgroup\$

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.