If you're using Boost, you could use the intrusive_ptr for this purpose.Quote:
Originally Posted by CornedBee
Printable View
If you're using Boost, you could use the intrusive_ptr for this purpose.Quote:
Originally Posted by CornedBee
:D
Indeed, I've wondered all the time.
>>Then you might be able to create a default behavior where a delete functor is called.
Different implementations of this have already been suggested by Zach and myself. I know mine works, though my compiler doesn't seem to handle Zach's version properly (unless it really isn't supposed to work).
And I really don't see why CornedBee's check/set NULL solution for the destructor problem wouldn't work?
**EDIT**
Huh? Wha? There IS only one reference count for each object, isn't there? :confused:Quote:
the problem is that you have several reference counts floating around for the same object, making it way too easy to destroy the object at the wrong time - you should have one and only one counter for each object.
His code looks okay. Make sure you have the space between the two > brackets. Another way might be to writeQuote:
Different implementations of this have already been suggested by Zach and myself. I know mine works, though my compiler doesn't seem to handle Zach's version properly (unless it really isn't supposed to work).
Code:template<typename T>
void generic_delete(T& ptr)
{
delete ptr;
ptr = 0;
}
template<typename T, void (*GD)(T& ptr) = generic_delete<T> >
class refptr {
//...//
};
If it's just a matter of someone purposely calling the destructor, I think then it's reasonable to release the resources. In fact, it's most likely what that person wanted. But for those who actually need to call the destructor without the reference decrementing, they could first increase the reference count and then call the destructor. Some type of addRef operation would have to be public, though.Quote:
And I really don't see why CornedBee's check/set NULL solution for the destructor problem wouldn't work?
>> I might be missing something, but it appears that the reference count will not properly be updated if you reassign the smart pointer.
>> If you do an assignment, the reference count of the original pointer never gets decremented (and accordingly, the pointer doesn't get freed
>> That's what I just pointed out. :)
>> You should be able to pass the onZeroRef function as a template...
I drove all the way home from work thinking, "something's not right.....well, they'll find it". The sad part is that I tried to model it after a known working implementation of mine but didn't look at operator=().
>> make the ZeroRef function a virtual member
I tend to use inheritance in a design as a last resort.
>> The user will be forced to use the explicit constructor even if you leave out this line.
>> Implement operator * and operator bool
<Cartman>"Sweeeeeeeeet".
A get() method seems fairly common too.
>> Also, perhaps the ZeroRef function itself would go well as a template parameter
Excellent idea [everyone]. I shy'd away from it at first since it changes the type of the smart pointer type. But on second thought, I like that since you wouldn't want to assign two smart pointers with different ZeroRef functors. Plus, it's more inline with the std:: way of doing it.
>> template<class T, class ZeroRefT = generic_delete<T> >
That would probably be the most likely usage scenario. I like it.
>> You could also create an internal templated ZeroRef function (protected, static), use that as the default [template] argument.
Yes, but the more std:: thing to do is to have a standalone unary_function<T, void> derivative.
>> the problem is that you have several reference counts floating around for the same object, making it way too easy to destroy the object at the wrong time
>> Huh? Wha? There IS only one ... isn't there?
I don't see the multiple reference counts either. Shall we chalk it up to smoke'n or drink'n or both? :D
>> the user could possibly mess up the count by calling ...destructors manually
I have to file that under: "if it hurts, don't do it". However, it should be mentioned in a proper tutorial ;) It could also be misused by calling the "ZeroRef" functor on a pointer manually. CornedBee and Sebastiani have nicely covered what not to do.
>> I'd use a Boost.Function object to store the deleter.
This seems like overkill to me since the fuctor's signature will always return void and take a pointer to T. If you need a different signature to be called, then using an adapter class is the more std:: way. This is also how boost::shared_ptr handles it.
Of course, for a truly robust implementation, use boost::shared_ptr for better:
- exception safety
- thread safety
- handling of different types of T
- debug'n facilities...and the list goes on...
Having said that, here are the changes for the new and improved version:
- used a more std:: naming style
- removed "using namespace std" to simulate the class being in a proper header (should be within a namespace as well)
- added operator*, operator bool, and T* get()
- added ref_count() member (rip off from boost::shared_ptr)
- made access methods (and operator bool) const
- made reference counting (more) correct
- "ZeroRef" functor is now a defaulted templated parameter
We should do this more ofter :)Code:#include <functional>
template<class T>
struct delete_ptr : public std::unary_function<T, void>
{
void operator()(T *&ptr) const {delete ptr; ptr = 0;}
};//delete_ptr
template <class T, class F = delete_ptr<T> >
class ref_ptr
{
protected:
F m_unload;
size_t *m_refs;
T *m_pointer;
// copy 'rcp' into '*this' and increment reference count
ref_ptr& copy(const ref_ptr &rcp)
{
if (m_refs && (m_refs == rcp.m_refs))
return *this;
m_refs = rcp.m_refs;
m_pointer = rcp.m_pointer;
(*m_refs)++;
return *this;
}//copy
// decrement reference count and call m_unload on 0
void dec_ref()
{
if (--(*m_refs) == 0)
{
delete m_refs;
m_refs = 0;
m_unload(m_pointer);
}//if
}//dec_ref
public:
// construction / destruction
explicit ref_ptr(T *p, const F &f = F())
: m_pointer(p), m_unload(f), m_refs(new size_t(1)) {}
~ref_ptr() {dec_ref();}
// copy semantics
ref_ptr(const ref_ptr &rcp) {copy(rcp);}
ref_ptr& operator=(const ref_ptr &rcp) {dec_ref(); return copy(rcp);}
// access
T& operator*() const {return *m_pointer;}
T* operator->() const {return m_pointer;}
T* get() const {return m_pointer;}
// conditional
operator bool() const {return m_pointer != 0;}
size_t ref_count() const {return m_refs ? *m_refs : 0;}
};//ref_ptr
//-----------------------------------------------------------------------------
#include <iostream>
#include <vector>
using namespace std;
struct A
{
A() {cout << "constructor" << endl;}
~A() {cout << "destructor" << endl;}
};//A
void DeleteA(A *p)
{
cout << "DeleteA - ";
delete p;
}//DeleteA
int main()
{
// use default "unload" functor
ref_ptr<A> rcp1(new A);
// use DeleteA() as the "unload" functor
ref_ptr<A, void(*)(A*)> rcp2(new A, &DeleteA);
vector<ref_ptr<A> > v1;
v1.push_back(rcp1);
v1.push_back(rcp1);
v1.push_back(rcp1);
v1.push_back(rcp1);
cout << "Ref Count = " << rcp1.ref_count() << endl;
vector<ref_ptr<A> > v2 = v1;
v2.erase(v2.begin());
v2.erase(v2.begin());
v2[0] = v2[1]; // <- brand new test case!!!
// should only be 2 constructions and destructions
return 0;
}//maim
gg
Well, it looks good.
If I must say something, I'd say make ref_count() const as well.
Why not? Just assign the deleter along with the pointer. Since the old pointer either gets deleted or released to the care of other smart pointers, you don't need the old deleter anymore. So you can just take a copy of the new one. (That's why Boost's shared_ptr stores the deleter in the same struct as the share count, btw.)Quote:
Originally Posted by Codeplug
Boost.Function isn't about different signatures (it's templated on the signature), but about different callable objects, such as function pointers, functors, ...Quote:
>> I'd use a Boost.Function object to store the deleter.
This seems like overkill to me since the fuctor's signature will always return void and take a pointer to T. If you need a different signature to be called, then using an adapter class is the more std:: way. This is also how boost::shared_ptr handles it.
The advantage would be that you could assign both of these as deleters:
Code:template<typename T>
void my_del_function(T *& p) {
delete p;
p = 0;
}
For getting both of these, you either have to template the smart pointer on the deleter type (as I explained above, I don't think that makes much sense) or use something akin to Boost.Function.Code:template<typename T>
struct my_del_functor
{
void operator()(T *& p) {
delete p;
p = 0;
}
};
>> I don't see the multiple reference counts either. Shall we chalk it up to smoke'n or drink'n or both?
nah, just me head up me arse. :D
Also, perhaps you could add a second 'built-in' delete functor, delete_array :) And that's where it becomes nice if you use the function pointer method: You can reassign a pointer-to-object to a pointer-to-array and vice versa, as long as the type you're pointing to is the same. I also wrote a [] operator, and scattered liberal asserts around to bugger the hell out of anyone who manual-destructs the thing several times and then tries copying it into another smartpointer.
The changes to dec_ref just ensure that even if the destructor is called manually, the reference count will only be decremented once.Code:void dec_ref()
{
if(m_refs == 0)
return;
if (--(*m_refs) == 0)
{
delete m_refs;
m_refs = 0;
m_unload(m_pointer);
}//if
m_refs = 0;
}//dec_ref
If the functor type is already locked in stone, why do you need to pass an object of it as an argument to the constructor? The functor specified in the template should always be the same as the one given to the constructor anyway.Code:explicit ref_ptr(T *p, const F &f = F())
Why do you need that? If both m_refs and rcp.m_refs are 0, then you won't end up returning at this point, and you'll dereference a NULL pointer.Code:if (m_refs && (m_refs == rcp.m_refs))
**EDIT** Oops, you'll end up dereferencing it anyway as long as rcp.m_refs == 0 and m_refs isn't. That's one of the spots that I assert()'ed myself on :) But the other thing is, it shouldn't matter if m_refs is NULL or not before you copy another ref_ptr; if it's NULL, then del_ref() will just do nothing (the pointer's already been taken care of, somehow), and then you'll make a copy of rcp, incrementing rcp's reference count, and life goes on.
Hunter:
delete m_refs;
m_refs = 0;
The second line is redundant, as it is executed unconditionally after the if anyway.
Wups. Actually, it was Codeplug's line, I just forgot to delete it when I moved it out of the if block ;)
**P.S.
What do all the const's do? I thought they just mean that the function won't be modifying member variables, and therefore allows them to be called on const objects of the class - but since operator-> etc. return T*, that allows them to modify the contents of the T pointed to, right? But isn't it a bad thing if a const ref_ptr is allowed to modify whatever it's pointing to? So perhaps it would be better to create two definitions of each operator:
T* operator->()
const T* operator->() const
T* get()
const T* get() const
etc.
This will break on self-assignment, won't it?Code:ref_ptr& operator=(const ref_ptr &rcp) {dec_ref(); return copy(rcp);}
Worse, it will break sometimes. It will work if dec_ref doesn't have safety precautions (such as setting to null) and there's still another smart pointer referring to the object.
Admittedly, the current version does have these safety precautions, so they will always break ;)
Not at all. AQuote:
But isn't it a bad thing if a const ref_ptr is allowed to modify whatever it's pointing to?
const ref_ptr<int>
is equivalent to a
int *const
.
Works. To have a pointer-to-const, pass a const type as template parameter.Code:int i = 0;
int *const p = &i;
*p = 5;
std::cout << i << std::endl;
ref_ptr<const int>
Hmm... is it really a good idea to disallow storage of null? Neither the std nor the boost smart pointers do it. Sometimes a null pointer can be really useful.
Edit: Uh, did someone just delete a post before this one?
Yup. I meant to get rid of all my overloaded const operators and repost it, but I'm going for a walk now ;)
**Pre-walk edit:
Sure it might not be a bad thing, but it'll take a little extra effort to handle that case. Otherwise you'll have reference counting for NULL :)