To get reference counting, do I have to clutter my APIs with shared_ptr?
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 ref开发者_如何学Goerence counting inside the Bar
and other objects yourself, using pimpl
and your own counters, but that gets too tedious as a general rule.
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.
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.
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.
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 somereturn
in theif
clutter ? You should move toward an object that manages memory for youauto_ptr
or the newunique_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 :)
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
There is no worrying if you follow your design and you understand what is weak_ptr
(hint: it isn't a reference) and what it is for (hint: it isn't for "breaking cycles").
If you do start worrying, it's because you never had a design, or you don't understand shared_ptr
design.
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
}
精彩评论