Jump to content
  • Advertisement
Sign in to follow this  
Prototype

Criticize my smart_ptr

This topic is 4077 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Because I'm trying to keep Boost out of my dependency list, I've been writing my own smart pointer lately. It's intended to be a reference counted pointer that behaves like an ordinary one, and allows for custom deleting. So far it works as expected but I would like you gurus to have a look and see if anything is wrong with it. Feel free to post other comments as well, as this is meant to be a learning experience. Here's the code:
// smart_ptr auto-deletion
enum delete_mode { delete_normal, delete_user };
        
// smart_ptr class
template <typename T, delete_mode Mode = delete_normal>
class smart_ptr
{
    private:
        // Local types
        typedef T* t_held;
        typedef smart_ptr<T, Mode> t_type;
        
        // Reference counted wrapper
        struct wrapped_ptr {
            t_held data;
            size_t ref_count;
            wrapped_ptr(const t_held& ptr) : data(ptr), ref_count(1) {}
        } *instance;
  
        // Ref counting
        void addref() { 
            if (instance) instance->ref_count++;
        }
        void release() {
            if ((instance) && (--instance->ref_count == 0)) {
                if (Mode == delete_normal) { delete instance->data; } 
                delete instance;
                instance = 0;
            }
        }
  
    public:
        /// Constructors ///
        smart_ptr() : instance(0) {} // default
        smart_ptr(const smart_ptr& that) { // copy constructor
            instance = that.instance;
            if (instance) addref();
        }
        smart_ptr(const t_held& that) { // init with pointer
            release();
            instance = new wrapped_ptr(that);
        }
        ~smart_ptr() { 
            release();
        }
  
        /// Operators ///
        t_held operator() () { // ..dereferencing
            return instance ? instance->data : 0;
        }
        t_held operator-> () { // ..dereferencing
            return instance ? instance->data : 0;
        }
        // ..cloning
        t_type& operator= (const t_type& that) {
            instance = that.instance;
            if (instance) addref(); // don't addref null pointers
            return *this;
        }
        // ..pointer assign
        t_type& operator= (const t_held& ptr) {
            release();
            instance = new wrapped_ptr(ptr);
            return *this;
        }
        // ..compare with smart_ptr
        bool operator== (const t_type& that) const {
            if (!instance) return false;
            return (instance == that.instance);
        }
        bool operator!= (const t_type& that) const {
            if (!instance) return false;
            return (instance != that.instance);
        }
        // ..compare with raw pointer
        bool operator== (const t_held& ptr) const {
            if (!instance) return false;
            return (instance->data == ptr);
        }
        bool operator!= (const t_held& ptr) const {
            if (!instance) return false;
            return (instance->data != ptr);
        }

        /// Functions ///
        bool Single() const { 
            return (instance->ref_count == 1);
        }
        bool Empty() const {
            return (instance->ref_count == 0);
        }
};

Some examples of usage:
// Use as a normal pointer
{
    smart_ptr<SomeClass> some = new SomeClass();
    some = new SomeClass(); // ok, deletes old object
    smart_ptr<SomeClass> other = some; // share pointer
    some->DoStuff(); // invoke member function
}
// .. pointer is deleted automatically
  
    
// Wrap an SDL_Surface
struct Surface {
    smart_ptr<SDL_Surface, delete_user> sdl_surface;
    //...
    ~Surface() { 
        if (sdl_surface.Single()) {
            SDL_FreeSurface(sdl_surface()); // get pointer with operator()
        }
    }
};

Your comments are greatly appreciated.

Share this post


Link to post
Share on other sites
Advertisement
- Reference counting is not thread-safe.

- It's not cross-assignable:

smart_ptr<SomeClass, delete_normal> s1;
smart_ptr<SomeClass, delete_user> s2;

s2 = s1; // will fail

void foo( smart_ptr<SomeClass> );

foo( s2 ); // will fail as well

This may seem like a minor point - but which version will you use in your API? There'll always be a case where it's the wrong one.

- Operator -> is overloaded, but operator* isn't, meaning that using this in templated code has a chance of failing in mysterious ways.

Share this post


Link to post
Share on other sites
Quote:
Original post by Antheus
- Reference counting is not thread-safe.

I know, but thread-safety is not a concern at this point. I'm not sure if sharing pointers across threads is a good idea in the first place.

Quote:

- It's not cross-assignable:
*** Source Snippet Removed ***This may seem like a minor point - but which version will you use in your API? There'll always be a case where it's the wrong one.

Basically they are different types, and should be treated as such. It's reasonable to expect that assigning an auto-delete pointer to a user-delete one will lead to trouble. The compiler nicely prevents this.

Quote:

- Operator -> is overloaded, but operator* isn't, meaning that using this in templated code has a chance of failing in mysterious ways.

Yup, forgot that one.
Thanks for taking the time.

Share this post


Link to post
Share on other sites
Do you have a really good and valid reason for not wanting to use boost? Smart ptr if I remember right is just a header file. It's also better and likely faster then yours. You're wasting time remaking already developed, tested and debugged tools instead of developing your application. If you use boost wisely, it will NOT bloat your code anymore then anything custom you write would.

Share this post


Link to post
Share on other sites
Quote:
Original post by Mike2343
Do you have a really good and valid reason for not wanting to use boost?

No. But I'm planning on releasing the code some day and I wouldn't like my users to download 12 other libraries just to compile it. Boost also tends to scare people away. My project currently has zero dependency and I cannot add a 23MB library for just a shared_ptr.

Share this post


Link to post
Share on other sites
I had reservations about using boost::shared_ptr as well, until I found out about 2 things. First, a pointer that looks and behaves exactly like boost::shared_ptr is already slated to be included in the next C++ standard (TR1). So, I figure if it's going to be standardized, I might as well start using it instead of rolling my own. Secondly, there is a nice boost utility called bcp which you can use to extract only the parts of boost that you need. In the case of shared_ptr, it will pull out the few header files necessary, and you can bundle those with your source. Quick, easy, painless, and you'll only need to change the namespace once shared_ptr becomes standardized.

Share this post


Link to post
Share on other sites
Quote:
Original post by Prototype
I'm planning on releasing the code some day and I wouldn't like my users to download 12 other libraries just to compile it.

It's trivial to distribute only the smart_ptr headers with your code.
Quote:
Boost also tends to scare people away.

Well, it scared you away, but I can't think of anyone else offhand.

Share this post


Link to post
Share on other sites
As I said, this is also a learning experience, and I'm just looking for constructive criticism to improve my understanding of C++. The reason I wrote it can be considered irrelevant. Don't take the discussion there please.

Share this post


Link to post
Share on other sites
Why does the constructor that takes a pointer call release()? It's a constructor, so instance is uninitialized at that time.

There is also a minor (or major, depending on how you look at it) exception handling problem. If new throws an exception when creating instance (because it ran out of memory), the wrapped pointer is never deleted. Making the users catch this exception and then delete the pointer themselves is extremely cumbersome. Boost's shared_ptr catches the exception (or checks if new succeeded by returning a non-zero value, if it's an environment where new doesn't throw exceptions), and then deletes the wrapped pointer before reraising or throwing an exception.

Boost also allows the user to specify a custom deleter.

One operator= doesn't release the previously held reference. The other operator= releases the reference before new is called, but new can throw an exception; switch the order so if new throws an exception the contents of the smart pointer are unchanged.

Hmm I need to stop posting, editing, getting distracted, then finish making modifications an hour later...

[Edited by - Vorpy on July 23, 2007 1:57:30 AM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Prototype
Basically they are different types, and should be treated as such. It's reasonable to expect that assigning an auto-delete pointer to a user-delete one will lead to trouble. The compiler nicely prevents this.


This is because your pointer does not allow custom deallocation: it only allows to choose between delete or no deallocation (at which point you have to handle it yourself).

A typical usage of a boost::shared_ptr is to return from a polymorphic factory, with some of the object types being deallocated with delete and others being returned to a memory pool. This would not be implementable using your method, because you cannot automatically return objects to a pool with it, and you certainly cannot mix pooled objects with non-pooled objects as a single return type, even though that would make sense.

As a side note, you're missing weak pointers, creation from this, and perhaps a void* operator for boolean contexts.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!