Re: ptr<X> versus const ptr<X>&
Davin Pearson wrote:
I have written a smart pointer class called ptr<X>
Here are two ways it could be used:
void foo(ptr<X> p)
{
// use p here
}
void foo(const ptr<X>& p)
{
ASSERT(&p != null);
// use p here
}
Um, why that assertion? If you have a reference, and taking the address of
that reference yields NULL, you have undefined behaviour (NULL pointer
dereference) already anyway!
Based on my work it seems that boths ways are equally efficient
because the size of the ptr<X> class is 4 bytes (enough to hold a
pointer variable) and therefore the variable can be passed inside a
register. Therefore it makes sense to use the first way because it is
simpler.
The copying overhead should be small anyway. The question is what it should
mean, e.g. passing a shared_ptr or an auto_ptr it would mean different
things.
The following code is the source code for my ptr<X> template
class:
template<class X>
class ptr
{
private:
X* inner_ptr;
public:
ptr()
{
inner_ptr = null;
}
- Members of a 'class' are private by default, no need to repeat it.
- Where is the definition of 'null'? BTW, ASSERT() is also not standard, it
is called assert() there!
- You should use initialisation instead of assignment.
ptr& operator = (const ptr& p)
{
ASSERT(this != null);
ASSERT(&p != null);
if (p.inner_ptr != null) // protect against s = s
{
p.inner_ptr->ref_count++;
}
if (inner_ptr != null)
{
assert(inner_ptr->ref_count > 0); // protect against
double deletions
inner_ptr->ref_count--;
if (inner_ptr->ref_count <= 0) // protect against case < 0
{
inner_ptr->ref_count = 1; // protect against
internal deletions
delete inner_ptr;
}
}
inner_ptr = p.inner_ptr;
return *this;
}
- Check for self-assignment is typically done like 'if(this!=&other)'.
- You could have written this much easier if you had just created a
temporary copy of 'p' and then swapped it with the asignee.
- I don't understand what you mean with 'internal deletions' and why you set
the refcouter to one again.
- I don't understand why you are checking for a negative refcount. Rather,
you should assert that this doesn't happen. You will have to use different
means than the increment/decrement operators when used in a multithreaded
environment tohugh.
- Prefer prefix increment to postfix increment.
ptr(X* x)
{
inner_ptr = x;
if (inner_ptr != null)
{
inner_ptr->ref_count++;
}
}
Consider making this one explicit to avoid implicit conversions from a
pointer to a ptr<X>. The problem occurs when you change a function:
void foo(X* px);
to
void foo( ptr<X> px);
If you used to call it like this:
X x;
foo(&x);
this will still compile but suddenly you have a ptr<X> taking ownership
of 'x' and deleting it when foo() returns. An explicit constructor makes it
mandatory to take according actions, i.e. increment the refcounter in
advance so that the ptr<X> doesn't reach zero.
ptr& operator = (X* x)
{[...]}
Same as other assignment operator overload: use a temporary and swap with
it. Obviously, you will need a swap function for that:
friend void swap( ptr& p1, ptr& p2) {
std::swap( p1.inner_ptr, p2.inner_ptr);
}
friend bool operator == (const ptr& p1, const ptr& p2)
{
if ((&p1 == null) && (&p2 == null))
{
return true;
}
else if ((&p1 == null) || (&p2 == null))
{
return false;
}
else
{
return (p1.inner_ptr == p2.inner_ptr);
}
}
You are checking the addresses of the ptr<X> objects here, but those must
not be null as explained above! The only thing this function should do is
return p1.inner_ptr == p2.inner_ptr;
...and then I'm pretty sure that GCC will optimise this pretty well.
friend bool operator != (const ptr& p1, const ptr& p2)
{
return !(p1 == p2);
}
No need to make this one a friend, it doesn't use any private parts. Note
that even operator== could have been implemented with just the public
interface.
One final note: boost::intrusive_ptr does pretty much all that your pointer
does and a bit more. If you intend yours for production code, I'd at least
take a look at that one for inspiration if not completely switch to it.
Uli
--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]