To get reference counting, do I have to clutter my APIs with shared_ptr?

Question

I recently had the following memory bug, which is easy to spot here, but can be harder to detect in more complex code:

class Foo : public IFoo {
  const Bar& bar_;
public:
  Foo(const Bar& bar) : bar_(bar) {
  }
  void test() {
    // access bar_ here
  }
};

int baz() {
  IFoo* foo = NULL;
  if(whatever) {
    Bar bar;
    foo = new Foo(bar);
  }
  else {
    // create other foo's
  }
  foo->test(); // segmentation fault
}

The bug is that Bar immediately goes out of scope, is destroyed and then used in foo->test(). One solution is to create Bar on the heap, using Bar* bar = new Bar(). However, I don't like to do this because I'd have to keep the Bar* bar pointer at top-level so I can access and delete it at the end, even though Bar is something that is specific to that particular code block if(whatever){}.

Another solution is boost::shared_ptr<Bar>, but I cannot just write this:

  if(whatever) {
    boost::shared_ptr<Bar> bar(new Bar());
    foo = new Foo(*bar);
  }

since the shared_ptr goes out of scope immediately, too, destroying the contained object.

So in short, to get rid of this problem, I'd have to use shared_ptr everywhere, in Foo as member variable, in Foo's constructor, etc. To eliminate these problems in general, all my APIs etc. have to use shared_ptr, which is kind of ugly. But, is it the right thing to do? So far, I have used it sometimes to create reference-counted objects, but I have kept my APIs clean of shared_ptr. How do you deal with this problem that, once you use shared_ptr you have to use it everywhere?

(Also, if you use these reference-counted pointers, you have to start worrying about if you really want shared_ptr or rather weak_ptr etc.)

And, what would I use as an equivalent to Foo(const Bar& bar)? Foo(const shared_ptr<const Bar> bar)?

Another option, of course, is to add reference counting inside the Bar and other objects yourself, using pimpl and your own counters, but that gets too tedious as a general rule.

Answer 1

It would be better to either not pass bar byref into Foo (in other words change Foos constructor) or copy it so you hold a duplicate in Foo. Of course in some cases this is not possible.

Regarding covering your code with shared_ptr<Bar>, the easiest solution is to typedef shared_ptr<Bar> to BarPtr or similar.

Answer 2

Actually I do use shared_ptr everywhere... There are several ways to make it look less cluttered. One convention I use is typedefs for each defined class:

class AClass {
public:
    typedef boost::shared_ptr<AClass> Ptr;
    typedef boost::weak_ptr<AClass> Ref;
    //...
};

Makes the code much more readable :)

As for your specific problem, remember that you can pass bar by pointer -- you'd have to remember to allocate it on the heap though.

Answer 3

You could simply move your bar object at a higher scope in order to prevent its deletion too soon.

int baz() {
  Bar bar;
  IFoo* foo = NULL;
  if(whatever) {
    foo = new Foo(bar);
  }
  else {
    // create other foo's
  }
  foo->test(); // no more segmentation fault
}

Answer 4

I understand that you provide a simplified version of your code, and without seeing the rest I don't know if what I am saying now is feasible.

Since foo is the longest-living object, how about creating the Bar instance inside it, and making other variables (bar in your code) references to that?

On the other hand, if Bar is really sepecific to the inside of the if block, test() should not need it at all. In this case, there is no problem in keeping a pointer inside Foo and then deleting it at the end of the if block. Otherwise, maybe it is not so specific to the block and you have to rethink your design a bit.

Answer 5

I would actually propose another solution...

I don't really like reference counting that much, not just the clutter, but also because it's harder to reason when multiple objects have a handle to the same item.

class IBar
{
public:
  virtual ~IBar() {}
  virtual IBar* clone() const;
};

class Foo: public IFoo
{
public:
  Foo(const IBar& ibar): m_bar(ibar.clone()) {}
  Foo(const Foo& rhs): m_bar(rhs.m_bar->clone()) {}
  Foo& operator=(const Foo& rhs) { Foo tmp(rhs); this->swap(tmp); return *this; }
  virtual ~Foo() { delete m_bar; }

  void swap(Foo& rhs) { std::swap(m_bar, rhs.m_bar); }

private:
  IBar* m_bar;
};

And now your code works, because I made a copy :)

Of course, if Bar was NOT polymorphic, then it would be much easier.

So, even though it's not using a reference, are you sure you actually need one ?

---

Let's get off track now, because there are a number of things you do wrong:

• foo = new Foo(bar) ain't pretty, how are you going to guarantee that the memory is deleted in case an exception occurs or somewhat introduces some return in the if clutter ? You should move toward an object that manages memory for you auto_ptr or the new unique_ptr from C++0x or Boost.
• instead of using reference counting, why does not Foo takes ownership ? Foo(std::auto_ptr<Bar> bar) would work too.

Shy from reference counting where it's not needed. Every time you share something you actually introduce a source of hard to track bugs in your code (and potentially side effects). That's why Haskell and the functional family are gaining popularity :)