6

I have a class following this pattern:

class Foo
{
public:
    // Create a Foo whose value is absolute
    Foo(int x) : other_(0), a_(x)  {}

    // Create a Foo whose value is relative to another Foo
    Foo(Foo * other, int dx) : other_(other), a_(dx) {}

    // Get the value
    double x() const
    {
        if(other_)
            return other_->x() + a_;
        else
            return a_;
    }

private:
    Foo * other_;
    int a_;
};

The Foo objects are all owned by a Bar:

class Bar
{
public:
    ~Bar() { for(int i=0; i<foos_.size(); i++) delete foos_[i]; }

private:
    vector<Foo*> foos_;
};

Of course, this is a simplified example to get the idea. I have a guarantee that there are no loop of Foos, and that linked Foos all belong to the same instance of Bar. So far, so good. To do things the C++11 way, I would use vector< unique_ptr<Foo> > foos_; in Bar, and pass foos_[i].get() as potential argument of a Foo constructor.

There is the deal:

This a GUI application, and the user can interactively delete some Foo at will. The expected behaviour is that if foo1 is deleted, and foo2 is relative to foo1, then foo2 becomes now "absolute":

void Foo::convertToAbsolute() { a_ += other_->x(); other_ = 0; }

void usageScenario()
{
    Foo * foo1 = new Foo(42);      
    Foo * foo2 = new Foo(foo1, 42);
    // Here, foo1->x() = 42 and foo2->x() = 84

    foo1->setX(10);
    // Here, foo1->x() = 10 and foo2->x() = 52

    delete foo1;
    // Here, foo2->x() = 52
}

I know it is possible to do it using raw pointers, by using a a DAG structure with back-pointers, so the Foo are aware of who "depends on them", and can inform them before deletion (possible solutions detailed here and here ).

My question is: Would you handle it the same way? Is there a way using standard C++11 smart pointers to avoid having the explicit back-pointers, and then avoid explicitely calling areRelativeToMe_[i]->convertToAbsolute(); in the destructor of Foo? I was thinking about weak_ptr, something in the spirit of:

class Foo { /* ... */ weak_ptr<Foo> other_; };

double Foo::x() const
{
    if(other_.isExpired())
        convertToAbsolute();

    // ...
}

But the issue is that convertToAbsolute() needs the relative Foo to still exist. So I need a non-owning smart-pointer that can tell "this reference is logically expired", but actually extends the lifetime of the referenced object, until it is not needed.

It could be seen either like a weak_ptr extending the lifetime until it is not shared with any other weak_ptr:

class Foo { /* ... */ extended_weak_ptr<Foo> other_; };

double Foo::x() const
{
    if(other_.isExpired())
    {
        convertToAbsolute();
        other_.reset(); // now the object is destructed,  unless other
                          // foos still have to release it
    }

    // ...
}

Or like a shared_ptr with different level of ownership:

class Bar { /* ... */ vector< multilevel_shared_ptr<Foo> foos_; };

class Foo { /* ... */ multilevel_shared_ptr<Foo> other_; };

void Bar::createFoos()
{ 
    // Bar owns the Foo* with the highest level of ownership "Level1"

    // Creating an absolute Foo
    foos_.push_back( multilevel_unique_ptr<Foo>(new Foo(42), Level1) );

    // Creating a relative Foo 
    foos_.push_back( multilevel_unique_ptr<Foo>(new Foo(foos_[0],7), Level1) );
}

Foo::Foo(const multilevel_unique_ptr<Foo> & other, int dx) :
    other_( other, Level2 ),
   // Foo owns the Foo* with the lowest level of ownership "Level2"
    a_(dx) 
{
}

double Foo::x() const
{
    if(other_.noLevel1Owner()) // returns true if not shared 
                               // with any Level1 owner
    {
        convertToAbsolute();
        other_.reset(); // now the object is destructed, unless 
                        // shared with other Level2 owners
    }
    // ...
}

Any thoughts?

Community
  • 1
  • 1
Boris Dalstein
  • 7,015
  • 4
  • 30
  • 59
  • 1
    "a non-owning reference that must be informed before the reference is destructed" - that seems like a perfect task for an *owning* reference. Are you sure this is not an XY problem? –  Sep 12 '13 at 21:41
  • @H2CO3 what's an XY problem? The idea is that I must still know when it is "conceptually expired", that shared_ptr cannot provide since, well... it is not expired if some other guy still owns it ;-) – Boris Dalstein Sep 12 '13 at 21:43
  • [The XY problem](http://meta.stackexchange.com/questions/66377/what-is-the-xy-problem). Well, I see. Perhaps you could use something like a delegate or notifications or something... I'm not great at using the C++ stdlib, just saying. –  Sep 12 '13 at 21:44
  • 1
    Ok, didn't know about this XY terminology ;-) My actual problem is indeed, when an object is deleted, to inform a dependent object that this deletion occurs. The "trivial" solution is using raw pointer and inform by hand dependent objects, as shown. I wondered if it was possible to do it using smart pointers. Thx anyway :-) – Boris Dalstein Sep 12 '13 at 21:49

5 Answers5

1

You can use the double link approach even with more than one other dependent object. You only have to link together the dependents of the same object:

class Foo {
public:
  explicit Foo(double x)
  : v(x), foot(nullptr), next(nullptr), dept(nullptr) {}

  // construct as relative object;  complexity O(1)
  Foo(Foo*f, double x)
  : v(x), foot(f), dept(nullptr)
  { foot->add_dept(this); }

  // destruct;  complexity  O(n_dept) + O(foot->n_dept)
  //                        O(1)  if !destroy_carefully
  ~Foo()
  {
    if(destroy_carefully) {
      for(Foo*p=dept; p;) {
        Foo*n=p->next;
        p->unroot();
        p=n;
      }
      if(foot) foot->remove_dept(this);
    }
  }

  double x() const
  { return foot? foot->x() + v : v; }

private:

  double v;   // my position relative to foot if non-null
  Foo*foot;   // my foot point
  Foo*next;   // next object with same foot point as me
  Foo*dept;   // first object with me as foot point

  // change to un-rooted;  complexity: O(1)
  void unroot()
  { v+=foot->x(); foot=nullptr; next=nullptr; }

  // add d to the linked list of dependents;  complexity O(1)
  void add_dept(const Foo*d)
  { d->next=dept; dept=d; }

  // remove d from the linked list of dependents ; complexity O(n_dept)
  void remove_dept(const Foo*d)
  {
    for(Foo*p=dept; p; p=p->next)
      if(p==d) { p=d->next; break; }
  }
  static bool destroy_carefully;

};
bool Foo::destroy_carefully = true;

Here, setting Foo::destroy_carefully=false allows you to delete all remaining objects without going through the untangling of mutual references (which can be expensive).

Walter
  • 44,150
  • 20
  • 113
  • 196
  • Oh, I am so sorry you went into the trouble of writting this! +1 for the time anyway. I *do know* how I would implement it using raw pointers and pretty confident on the details and how it would work ("The simplest raw pointer approach I can think of is to have a two-way link [...]"). My question was raelly more: "Would you do the same thing (then a *yes* is enough), or is there an alternative using smart pointerd?". Should have make this clearer I guess. – Boris Dalstein Sep 12 '13 at 22:51
  • No problem, was fun. It's a three-way link, not a two-way link. I was mainly spurred to disprove David's answer. – Walter Sep 12 '13 at 22:53
  • Hehe, your code gave me an headache but I think is correct after a few drawings executing the methods :-D But you actually don't need "next", a two-way link (parent-child) is enough, no need to order them. (I'll write it as an answer). The added information doesn't even give you any performance gain since anyway your `remove_dept` is linear in number of dependent objects. Worse, It's actually less efficient in number of RAM access since you have to maintain the `next` consistency. – Boris Dalstein Sep 12 '13 at 23:12
  • @Boris Without the `next`? How can you then unroot all dependents if an object gets destroyed? It has to find *all* other objects with `other->foot==this`. – Walter Sep 12 '13 at 23:23
  • @Walter: This is clearly *not* a double linked list by any standard. But rather a *tree* with back pointers. One of the common implementations of a variable arity tree is holding a pointer to the first descendant, and a list of siblings with an optional backpointer to the parent. In this case the tree is rooted at the `foot` (backpointer to parent), `next` is the singly linked list of siblings and `dept` is the pointer to the first child. This clearly does not disprove that it cannot be done with a doubly linked list. – David Rodríguez - dribeas Sep 13 '13 at 01:21
  • @Boris: You need the `next` pointer since the relationship is 1:N. Also note that this **is** way more efficient than your proposed solution. If for nothing else because the data structure is *intrusive* and there are less hops **and** you will need roughly half of the memory allocations that your solution does. As a matter of fact if you are going to use an external container to hold the pointers, the natural choice would be `std::vector<>` since there is no natural order for the elements. From a theoretical point of view a `std::list` would be better, from a practical point it isn't. – David Rodríguez - dribeas Sep 13 '13 at 01:25
  • @David Thx for the clarifications. I think no-one was talking about a double-linked *list*, the "double-link" thing was a (probably wrong) terminology to what you describe: back-pointers (actually a DAG with back-pointers: there might be loops if seen as undirected, e.g. a diamond). Anyway, I'm using Qt and implement it using my method using a `QVector` for now: at equal complexity, I don't care about efficiency unless observing it's an issue. Readability is the priority for me in this case. – Boris Dalstein Sep 13 '13 at 02:17
1

Interesting problem. I guess you figured that you can add a pointer to the 'child' object. I am not sure, whether smart pointers help here. I tried to implement the code below using std::weak_ptr<Foo> but you can only use it for other_ and not for the listener.

Another thought I had was to leave the responsibility to some higher power. The problem that you have is that you would like to do the update when the destructor is called. Perhaps better approach would be to call convertToAbsolute() from somewhere else. For example, if you are storing the Foos in a vector and the user clicks delete in the UI, you need the index of the object in order to delete so might as well update the adjacent item to absolute value.

Below is a solution that uses a Foo*.

#include <iostream>
#include <memory>
#include <vector>


class Foo
{
public:
    // Create a Foo whose value is absolute
    Foo(int x) : other_(nullptr), listener_(nullptr), a_(x)
    {}

    // Create a Foo whose value is relative to another Foo
    Foo(Foo* other, int dx) : 
    other_(other), listener_(nullptr), a_(dx) 
    {
        other->setListener(this);
    }

    ~Foo()
    {
        convertToAbsolute();
        if (listener_)
            listener_->other_ = nullptr;
    }

    // Get the value
    double x() const
    {
        if(other_)
            return other_->x() + a_;
        else
            return a_;
    }

    void setX(int i)
    {
        a_ = i;
    }

    void convertToAbsolute()
    {
        if (listener_)
            listener_->a_ += a_;
    }

    void setListener(Foo* listener)
    {
        listener_ = listener;
    }

private:
    Foo* other_;
    Foo* listener_;
    int a_;
};


void printFoos(const std::vector<std::shared_ptr<Foo>>& foos)
{
    std::cout << "Printing foos:\n";
    for(const auto& f : foos)
        std::cout << '\t' << f->x() << '\n';
}

int main(int argc, const char** argv)
{
    std::vector<std::shared_ptr<Foo>> foos;
    try
    {
        auto foo1 = std::make_shared<Foo>(42);
        auto foo2 = std::make_shared<Foo>(foo1.get(), 42);

        foos.emplace_back(foo1);
        foos.emplace_back(foo2);
    }
    catch (std::exception& e)
    {
        std::cerr << e.what() << '\n';
    }

    // Here, foo1->x() = 42 and foo2->x() = 84
    printFoos(foos);

    foos[0]->setX(10);
    // Here, foo1->x() = 10 and foo2->x() = 52
    printFoos(foos);

    foos.erase(foos.begin());
    // Here, foo2->x() = 52
    printFoos(foos);

    return 0;
}
Roman Kutlak
  • 2,684
  • 1
  • 19
  • 24
  • So indeed, using `shared_ptr` or `unique_ptr` in the caller code makes things cleaner caller-side, but my issue was really in `Foo` side. I was wondering if a smart pointer could be used for `other_` to delay its destruction, avoiding the whole "listener" thing. It seems the answer is no using the standard smart pointers, and one would need implementing its own extended smart pointer that I describe. Your help was much appreciated though :-) – Boris Dalstein Sep 13 '13 at 00:46
  • I think the more I think about the problem, the more interesting it gets. I think only when I started to write the solution I realised what the problem was. And only after solving it I understood what the question really is :-) Anyway, I updated the answer by adding an extra thought. – Roman Kutlak Sep 13 '13 at 17:31
  • Yep, moving the responsibility of deleting cleanly to a higher power would definitely be a good architecture design. :-) My concern is that it is a research project, so you need fast iterations to debug ideas, and it's hard to plan architecture in advance. Hence as a *guideline*, I favor self-contained safe classes rather than safe global architecture. It makes it faster to prototype and shuffle things around :-) For now, I'm going for the raw back-pointers to get things done, but will probably play around with handmade extended smart pointers later, cause it looks fun ;-) – Boris Dalstein Sep 13 '13 at 20:34
1

All Foo are owned by Bar. Therefore all deletions of Foo happen in Bar methods. So I might implement this logic inside Bar:

void Bar::remove(Foo* f)
{
    using namespace std::placeholders;
    assert(std::any_of(begin(foos_), end(foos_),
                       std::bind(std::equal_to<decltype(f)>(), f, _1));

    auto const& children = /* some code which determines which other Foo depend on f */;
    std::for_each(begin(children), end(children),
                  std::mem_fn(&Foo::convertToAbsolute));
    foos_.remove(f);

    delete f; // not needed if using smart ptrs
}

This would ensure that the expiring Foo still exists when convertToAbsolute is called on its dependents.

The choice of how to compute children is up to you. I would probably have each Foo keep track of its own children (cyclic non-owning pointers), but you could also keep track of it inside Bar, or search through foos_ on demand to recompute it when needed.

Oktalist
  • 14,336
  • 3
  • 43
  • 63
  • +1, this bugged me and was helpful: "All Foo are owned by Bar. Therefore all deletions of Foo happen in Bar methods.". Is this true under most accepted definitions of ownership? Couldn't a non-owning holder delete a reference? If yes then no, then I was wrong stating that `Bar` *owns* the `Foo`. I would like that an external class with a pointer of `Foo` could delete it. Oh, but now I'm thinking! Maybe this may be done not by "deleting" the object, but calling `foo->destroy()` (e.g., from a weak_ptr) , that delegates the destruction to the owning `Bar`, so your approach is still valid? – Boris Dalstein Sep 13 '13 at 21:59
  • 1
    @Boris All definitions of ownership that I have come across include responsibility for deletion. Although it's reasonable to permit a "transfer of ownership" from one owner to another. The main thing you need to make sure of is that each `Foo` is deleted exactly once. If some other code deletes a `Foo`, then you need to remove it from the `Bar` anyway. `foo->destroy()` sounds like a good solution. – Oktalist Sep 14 '13 at 00:04
  • 1
    Furthermore, bare `delete` is a low-level primitive memory-management mechanism, so trying to use it to signal changes in high-level logical relationships was probably not the best idea. – Oktalist Sep 14 '13 at 13:43
  • Yep, this finally made a lot of sense. It made me realize that indeed it is `Bar` responsibility to do this and I can't really move it to `Foo`, since anyway `Bar` must be aware of the deletion to keep the data structure sound. Thx! – Boris Dalstein Sep 16 '13 at 04:43
  • One more thing: re the answers which rely on doing something in a destructor (including any smart pointer based solution), remember you should make sure that [destructors don't throw exceptions](http://stackoverflow.com/q/130117/1639256). – Oktalist Sep 16 '13 at 14:01
1

If you have a Signal/Slot framework, that provides a nice place to do the unlinking. For example, using the Qt libraries these classes could look like:

class Foo : public QObject
{
Q_OBJECT
public:
    // Create a Foo whose value is absolute
    Foo(int x) : QObject(nullptr), other_(nullptr), a_(x) {}

    // Create a Foo whose value is relative to another Foo
    Foo(Foo * other, int dx) : QObject(nullptr) other_(other), a_(dx) {
        connect(other, SIGNAL(foo_dying()), this, SLOT(make_absolute()));
    }

    ~Foo() { emit foo_dying(); }

    // Get the value
    double x() const
    {
        if(other_)
            return other_->x() + a_;
        else
            return a_;
    }

signals:
    void foo_dying();

private slots:
    void make_absolute()
    {
        a_ += other_->x();
        other_ = nullptr;
    }

private:
    Foo * other_;
    int a_;
};
aschepler
  • 70,891
  • 9
  • 107
  • 161
  • Oh, that is a wonderful solution, I wonder why I didn't think about it! Just some notes: 1) in fact, the signal must be `void foo_dying(Foo *);` then `emit foo_dying(this);` for irrelevant reasons you couldn't guess with my simplified example. 2) If anyone read this, make sure your connection type is `Qt::DirectConnection` (then doesn't work across threads). 3) Too bad we can't use the built-in signal `QObject::destroyed()` since it is called in `~QObject()`, and hence `Foo::x()` is not available anymore. Otherwise, it would even be possible not to define `~Foo` at all! – Boris Dalstein Sep 13 '13 at 23:53
  • 4) Again if anyone read this, keep in mind that this has [quite a big memory overhead](http://stackoverflow.com/questions/15763088/how-heavy-is-qobject-really), but not that much time overhead I would guess. – Boris Dalstein Sep 13 '13 at 23:54
  • Yeah, I was first going to write it as using `destroyed()` but realized that wouldn't work. – aschepler Sep 14 '13 at 04:51
0

Here is probably the simplest way to achieve the goal using back-pointers. You can use the container you want depending on your complexity requirements (e.g., a set, hash table, vector, linked list, etc.). A more involved but more efficient approach is proposed by Walter.

class Foo
{
public:
    // Create a Foo whose value is absolute
    Foo(int x) : other_(0), a_(x)  {}

    // Create a Foo whose value is relative to another Foo
    Foo(Foo * other, int dx) : other_(other), a_(dx)
    {
        other->areRelativeToMe_.insert(this);
    }

    // Get the value
    double x() const
    {
        if(other_)
            return other_->x() + a_;
        else
            return a_;
    }

    // delete the Foo
    Foo::~Foo()
    {
        // Inform the one I depend on, if any, that I'm getting destroyed
        if(other_)
            other_->areRelativeToMe_.remove(this);

        // Inform people that depends on me that I'm getting destructed
        for(int i=0; i<areRelativeToMe_.size(); i++)
            areRelativeToMe_[i]->convertToAbsolute();
    }

private:
    Foo * other_;
    int a_;
    Container<Foo*> areRelativeToMe_; // must provide insert(Foo*) 
                                      //          and remove(Foo*)

    // Convert to absolute
    void convertToAbsolute()
    {
        a_ += other_->x(); 
        other_ = 0; 
    }
};
Community
  • 1
  • 1
Boris Dalstein
  • 7,015
  • 4
  • 30
  • 59
  • Instead of two pointers (`dept` and `next`) you maintain a complete `std::set` for the dependents. This requires more RAM and is slower (logarithmic complexity upon insertion). – Walter Sep 12 '13 at 23:35
  • @Walter: AND logarithmic complexity upon deletion too. Your solution use linear time to delete it. ;-) It depends on what you want. Anyway, you can use a list instead of a set, so you have the same complexity as you, but much simpler to program. (I mean, it is the same algorithm executed underneath, but let the list do it for you ;-) ) – Boris Dalstein Sep 12 '13 at 23:39
  • @Walter Actually, at first I used a vector. But realize the std::vector is stupid enough not to provide a (linear time) `erase(const T &)` method, so the presentation was cleaner using a set. – Boris Dalstein Sep 12 '13 at 23:42
  • ok, you have faster removal from the set, but at a high price (RAM wise) and without actually achieving much, as the destructor is still linear in the number of dependents (of the object destroyed, not of the footpoint). It would require testing to see whether there is any benefit; I suspect only for very large numbers of dependents which are all destructed before their footpoint will the `set` pay off. – Walter Sep 12 '13 at 23:45
  • @Walter I just had another headache ;-) Your approach is indeed more RAM-economic that mine, but not a lot. In your approach, every object have two pointers `next` and `dept`, so the RAM cost is **2**. In my approach, every object has one list (to keep the same time complexity), i.e. a pointer to a list iterator (null if no dept). Each iterator takes 2 bytes in memory (a pointer to the pointed element, and a pointer to next). Because each `Foo` has *at most* one list-iterator pointing to him (they belong to only one list: their foot), you can count as 2 bytes per `Foo`. So final cost: **3** – Boris Dalstein Sep 13 '13 at 00:02
  • @Walter In conclusion, since the object is anyway much heavier than this (at least 10 bytes), saving one byte per object is not worth the complexity of the source code, and I would go for the simplest approach. But yes, embedding the list directly in the structure as you did *indeed is* more efficient (just not worth the trouble/readability). – Boris Dalstein Sep 13 '13 at 00:04
  • 1
    *one byte* -- on what kind machine are you with pointers (let along `std::set`) of this size? What is worth what is, of course, a matter of taste, but readability can be helped with comments, as I have tried. – Walter Sep 13 '13 at 00:21
  • @Walter Oops, sorry, indeed, I meant 32bits of 64bits for the address, instead of "Byte". Plus of course, an additional overhead if you want to cache things like `size`, etc. The idea here is just that it "only" adds a constant RAM space per `Foo` (the size of a "list" + the size of a "list-iterator", which is in my case a small fraction than the size of my objects (and even if it tripled the size of my object it would still be fine). I was worried that the space *complexity* actually changed, dramatically increasing RAM usage. But it doesn't, it is still linear in the number of `Foo`. – Boris Dalstein Sep 13 '13 at 00:28
  • @Boris: *`std::vector` is stupid enough not to provide a (linear time) `erase()`*. First, erasing from the vector **is** linear time, maybe you meant *constant time* or *logarithmic time*?. Even there, for a few thousand elements of pointer size, the linear time operation on `vector` will outperform the equivalent operation on `std::list` in any current processor. Asymptotic notation hides the real costs and deals only with the relationship between the cost and the size of the problem as the problem grows. – David Rodríguez - dribeas Sep 13 '13 at 01:32
  • @Boris: the cost of the extra memory is not really the memory (4x that needed by a vector), but the number of distinct allocations/deallocations (1 per object, depending on the implementation 1 extra per set), and the lack of locality of data. – David Rodríguez - dribeas Sep 13 '13 at 01:34
  • @David Thx for pointing this out that the issue is more about allocation and locality of data than memory space. This convinced me to use `QVector` over `QSet`, even `QSet` made more sense in a readability perspective since it represents more what it is: an unordered set of unique pointers (a vector gives an implicit ordering and is by design able to store several times the same value). – Boris Dalstein Sep 13 '13 at 02:21
  • @David, Oh and I forgot about the `std::vector::erase` thing. I was just saying that I think `std::vector` *should provide the method*. Then it would be linear of course, but this is not a reason not to make it part of the interface. I don't understand why I have to write myself such a fundamental operation for a container, and call `erase(vec, elt)` instead of `vec.erase(elt)`. – Boris Dalstein Sep 13 '13 at 02:37
  • @Boris: on the contrary, there is a very good reason: it can be implemented as efficiently as it is done now, with a combination of the external function `std::find` and the member function `erase`. There is no need to pollute the interface of all containers if there is no clear advantage in implementing it as a member. `auto it = std::find(v.begin(),v.end(),value); if (it != v.end()) v.erase(it);`. If you know that the element is there, then skip the `if`: `v.erase(std::find(v.begin(),v.end(),value));` – David Rodríguez - dribeas Sep 13 '13 at 02:54
  • @David, I think we can agree to disagree on this point ;-) I see a world between all the characters you used and `v.erase(value)`, and in my opinion it is worth the very limited pollution. – Boris Dalstein Sep 13 '13 at 03:00
  • @Boris: We can agree to disagree... and we can also agree that you disagree with Stepanov that designed the library and most of the standard committee. – David Rodríguez - dribeas Sep 13 '13 at 03:06
  • @David then yes, I disagree with this very respectable guy on this very specific point ;-) – Boris Dalstein Sep 13 '13 at 03:07
  • @David I just googled and found [one answer of your own](http://stackoverflow.com/questions/6447106/why-vector-doesnt-provide-the-remove-member-function-while-list-provides) ;-) Nice explanation, but I still feel (for now, I can change my mind...) that not being able to program it more efficiently as a member shouldn't restrain from providing it as a convenience (especially if it doesn't impact negatively the other methods). – Boris Dalstein Sep 13 '13 at 03:15