Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The lifetimes of various objects referring to each other are sometimes only known at runtime. For example, in a side scrolling shooter game, a HomingMissile instance might have a pointer to the Target instance it is following. But if the Target is killed before the missile arrives, the HomingMissile is left with a dangling pointer to where the Target was in memory, likely causing a segmentation fault.

It would be nice if pointers could just be checked for validity, but it is obvious that the following example for detecting wether or not ptrToFoo is valid by comparison with nullptr will fail, as a pointer is merely an address, having no knowledge about the lifetime of the pointed object.

struct Foo
{
    const char *hello() const
    {
        return "hello world";
    }
};

int main()
{
    Foo *foo = new Foo;
    Foo *ptrToFoo = foo;
    std::cout << ((ptrToFoo == nullptr) ? "expired!" : ptrToFoo->hello()) << "\n";
    delete foo;
    std::cout << ((ptrToFoo == nullptr) ? "expired!" : ptrToFoo->hello()) << "\n";
    return 0;
}
//output:
//hello world
//hello world     <- bad

C++11 smart pointers are great, but they do not feel like the right tool for the job. It makes no sense for the HomingMissile to own the Target trough a std::shared_ptr or a std::unique_ptr. Having HomingMissile hold a std::weak_ptr to the Target does allow checking the state of the target instance, but it forces the Target to be owned trough a std::shared_ptr by some unrelated third party. In addition, constantly calling weak_ptr::lock() is cumbersome at least.

As a solution, I have been writing a different kind of smart pointer, one that does not imply any kind of ownership at all. The idea is to have a reference-count-based pointer-like class that knows if the pointed object has been destroyed or not, preventing the "dangling pointer" problem:

struct Foo : public ObserverPtr<Foo>::Observable
{
    const char *hello() const
    {
        return "hello world";
    }
};

int main()
{
    Foo *foo = new Foo;
    ObserverPtr<Foo> ptrToFoo(*foo);
    std::cout << (ptrToFoo.expired() ? "expired!" : ptrToFoo->hello()) << "\n";
    delete foo;
    std::cout << (ptrToFoo.expired() ? "expired!" : ptrToFoo->hello()) << "\n";
    return 0;
}
//output:
//hello world
//expired!

The ObserverPtr implementation should work for all classes derived from ObserverPtr</*derived class name*/>::Observable, independently of allocation type (stack, heap, gobal) or any type qualifiers (const, volatile). Multithreading support is not required at this stage.


Implementation of ObserverPtr:

template<class T>
class ObserverPtr
{
public:
    class Observable
    {
    protected:
        ///////////////////////////////////////////////////////////////////////
        // Reset all ObserverPtrs that point to this ObserverPtr::Observable-
        // ..derived object.
        void resetObserverPtrs()
        {
            if(tracker_->referenceCount > 1)
            {
                tracker_->pointer = nullptr;
                tracker_->referenceCount--;
                tracker_ = new Tracker {static_cast<T*>(this), 1};
            }
        }
        //The constructor and destructor are protected. This ensures that the
        //..class ObserverPtr::Observable cannot exist on it's own but must be
        //..inherited.
        Observable()
            : tracker_(new Tracker {static_cast<T*>(this), 1})
        { }
        ~Observable()
        {
            if(tracker_->referenceCount > 1)
            {
                tracker_->pointer = nullptr;
                tracker_->referenceCount--;
            }
            else
                delete tracker_;
        }
    private:
        //The Tracker is a reference counted, heap allocated object that
        //..is used internally by ObserverPtr system for tracking the state 
        //..of the pointed object.
        struct Tracker
        {
            T *pointer;
            unsigned int referenceCount;
        };

        Tracker* tracker_;
        friend ObserverPtr;
    };

    ///////////////////////////////////////////////////////////////////////////
    // Construct a blank ObserverPtr that points to or observes nothing
    ObserverPtr()
        : tracker_(nullptr)
    { }

    ///////////////////////////////////////////////////////////////////////////
    // Construct an ObserverPtr to observe an observable class.
    ObserverPtr(T &observed)
        : tracker_(observed.tracker_)
    {
        tracker_->referenceCount++;
    }

    ///////////////////////////////////////////////////////////////////////////
    // Construct an ObserverPtr as a copy of another ObserverPtr
    ObserverPtr(const ObserverPtr &other)
        : tracker_(other.tracker_)
    {
        tracker_->referenceCount++;
    }

    //TODO: Move constructor

    ///////////////////////////////////////////////////////////////////////////
    // Destructor.
    ~ObserverPtr()
    {
        reset();
    }

    ///////////////////////////////////////////////////////////////////////////
    // Resets the the ObserverPtr to observe nothing.
    void reset()
    {
        if(tracker_ != nullptr)
        {
            if(tracker_->referenceCount > 1)
                tracker_->referenceCount--;
            else
                delete tracker_;
            tracker_ = nullptr;
        }
    }

    ///////////////////////////////////////////////////////////////////////////
    // Reset the ObserverPtr to point to and observe a different object
    void reset(const T &observed)
    {
        if(tracker_ != observed.tracker_) //prevent resetting to itself
        {
            reset();
            tracker_ = observed.tracker_;
            if(tracker_ != nullptr)
            {
                tracker_->referenceCount++;
            }
        }
    }

    ///////////////////////////////////////////////////////////////////////////
    // Reset the ObserverPtr to observe nothing.
    ObserverPtr &operator=(std::nullptr_t)
    {
        reset();
        return *this;
    }

    ///////////////////////////////////////////////////////////////////////////
    // Reset the ObserverPtr observe the same object as another ObserverPtr
    ObserverPtr &operator=(const ObserverPtr &other)
    {
        if(tracker_ != other.tracker_) //prevent self-assignment
        {
            reset();
            tracker_ = other.tracker_;
            if(tracker_ != nullptr)
            {
                tracker_->referenceCount++;
            }
        }
        return *this;
    }

    //TODO: Move assignment operator

    ///////////////////////////////////////////////////////////////////////////
    // Swap the observed objects between two ObserverPtrs
    void swap(ObserverPtr &other)
    {
        typename ObserverPtr::Observable::Tracker *previousSentry = tracker_;
        tracker_ = other.tracker_;
        other.tracker_ = previousSentry;
    }

    ///////////////////////////////////////////////////////////////////////////
    // Dereference member of the object pointed by ObserverPtr.
    // ..Calling this on a expired ObserverPtr results in undefined behaviour!
    T *operator->() const
    {
        return tracker_->pointer;
    }

    ///////////////////////////////////////////////////////////////////////////
    // Dereference the ObserverPtr. Returns a reference to the pointed object.
    // ..Calling this on a expired ObserverPtr results in undefined behaviour!
    T &operator*() const
    {
        return *(tracker_->pointer);
    }

    ///////////////////////////////////////////////////////////////////////////
    // Dereference the ObserverPtr. Returns a reference to the pointed object.
    // ..Calling this on a expired ObserverPtr results in undefined behaviour!
    T *get() const
    {
        return tracker_->pointer;
    }

    ///////////////////////////////////////////////////////////////////////////
    // Return true if the ObserverPtr is expired. Returns true if the observed
    // ..object has been destroyed, or if the ObserverPtr has been constructed
    // ..or reset to observe nothing.
    bool expired() const
    {
        if(tracker_ == nullptr)
            return true;
        if(tracker_->pointer == nullptr)
            return true;
        return false;
    }

    ///////////////////////////////////////////////////////////////////////////
    // Return true if the ObserverPtr is expired. Equivalent to expired().
    bool operator==(std::nullptr_t) const
    {
        return expired();
    }

    ///////////////////////////////////////////////////////////////////////////
    // Return true if the ObserverPtr is not expired. Equivalent to !expired().
    bool operator!=(std::nullptr_t) const
    {
        return !expired();
    }

    ///////////////////////////////////////////////////////////////////////////
    // Test two ObserverPtrs for equality. Returns true if both pointers point
    // ..to the same object or if both pointers are expired.
    bool operator==(const ObserverPtr &other) const
    {
        if(expired())
            return other.expired();
        else
        {
            if(other.expired())
                return false;
            else
                return tracker_->pointer == other.tracker_->pointer;
        }
    }

    ///////////////////////////////////////////////////////////////////////////
    // Test two ObserverPtrs for inequality. Returns false if both pointers
    // ..point to the same object or if both pointers are expired.
    bool operator!=(const ObserverPtr &other) const
    {
        return !((*this) == other);
    }

private:
    typename Observable::Tracker *tracker_;
};

The implementation is still work in progress:

  • A move constructor has not yet been implemented.

  • A move assignment operator has not yet been implemented.

  • Implicit type conversions between ObserverPtrs of different types are not supported, and I think that it should stay that way. Possibly an overload of std::static_pointer_cast should be provided.

  • An ObserverPtr to a const- declared object (ObserverPtr<const Foo>) does not currently compile.

  • The code has not yet been troughoutly tested.

  • The code is at this stage completely unoptimized.

  • I am still relatively new to C++ and especially C++11, so the code may not be as good as it could be, and writing a robust smart pointer is surprisingly difficult.

I would like to receive feedback on the concept in general and my implementation. I currently cannot figure out how to add support for pointing to const objects, but I am working on it.

share|improve this question
    
I'm surprised that the std pointers aren't good enough. Target has to be owned by something which controls its lifetime. If you don't want to have Target in a shared_ptr and give Missile a weak_ptr, then why not have Target in a unique_ptr and give Missile a simple reference to the unique_ptr? –  ChrisW Mar 21 at 23:53
1  
@ChrisW I disagree with that solution, since there would be many missiles and targets that are all stored (and owned by) a single vector of std::unique_ptrs. What happens when a target gets destroyed? is the manager holding the vector forced to keep a null unique_ptr in the vector indefinitely, in case a missile checks if the unique_ptr in the vector is null? even a linked list wouldn't help. –  jms Mar 22 at 0:07
    
Why don't you use a shared_ptr for Target and weak_ptr in Missile? That allows the shared_ptr to be destroyed. –  ChrisW Mar 22 at 0:22
1  
I won't post this as an answer then, but as a comment: you can use standard shared_ptr and weak_ptr instead of 'reinventing the wheel'. Your ObserverPtr is analogous to a weak_ptr, and your ObserverPtr::Observable is analogous to a shared_ptr. –  ChrisW Mar 22 at 0:43
1  
You have edited your post 8 or 9 times. If you edit it more than 10 times it will be auto-converted to Community Wiki, which may remove people's incentive to answer it: so perhaps stop editing it now. –  ChrisW Mar 22 at 0:48
show 3 more comments

2 Answers 2

up vote 5 down vote accepted
Foo *foo = new Foo;

This isn't safe: an exception later on will leak the object.

///////////////////////////////////////////////////////////////////////

These waste vertical space which makes to harder to read.

void resetObserverPtrs()

This isn't called from anywhere.

void reset(const T &observed)

This isn't called from anywhere; but it is public. Perhaps it should be an assignment operator=.

I'm not sure why you have if(tracker_ != nullptr) in this method: IMO observed.tracker_ can never be null (unless observed has been destroyed, in which case it should not have been passed as a parameter to this method).

void swap(ObserverPtr &other)

Maybe use std::swap to implement this method.

T *get() const

Instead of undefined behaviour, if (expired()) return nullptr;.


Also, you can use standard shared_ptr and weak_ptr instead of 'reinventing the wheel'. Your ObserverPtr is analogous to a weak_ptr, and your ObserverPtr::Observable is analogous to a shared_ptr.


However, while Foo *foo = new Foo; is not safe, does it really matter?

It's up to you whether it matters: see this meta-answer for details.

It's just a short usage example

Then it's an example of unsafe usage, is all I'm saying. What questions are on-topic for this site? says, "Is it actual code from a project rather than pseudo-code or example code?"

You also said, "The ObserverPtr implementation should work ... independently of allocation type (stack, heap, gobal)" and said you don't want to use standard smart pointers: so maybe you're intending to use Foo* in your real code as well as in your example code.

Another problem, which I didn't mention previously, is that because Observable contains a naked Tracker* pointer, you should delete or override the copy constructor and assignment operator for Observable. Deleting those will make it difficult to store Observable instances in standard containers. You can store naked pointers to Observable instances (e.g. std::list<Foo*> though not std::list<Foo>, but naked pointers aren't exception safe.

share|improve this answer
    
Point taken, the two places where new Tracker {static_cast<T*>(this), 1} can be fould should be new(std::nothrow) Tracker {static_cast<T*>(this), 1} instead. However, while Foo *foo = new Foo; is not safe, does it really matter? It's just a short usage example. –  jms Mar 22 at 2:39
    
I agree with the other ones. The reason why there is reset(const T &observed) instead of operator=(const T &observed) is that I just copied the style of std::shared_ptr where there is the distinction that reset is used for releasing ownership and possibly taking ownership of a previously unmanaged object while operator= is used for releasing ownership and possibly transferring ownership of objects between pointers. –  jms Mar 22 at 2:49
    
@user1062874 I updated my answer to reply to your question. –  ChrisW Mar 22 at 3:01
    
"Point taken, ..." -- After you create a Foo* instance as a local variable, if any of the next statements throw then the Foo* instance is leaked. That's a reason for using smart pointers. –  ChrisW Mar 22 at 3:09
    
While the part of the code I have submitted for review is intended to be the implemtation of ObserverPtr, not the usage example, I do agree that I am promoting bad style in the usage example, and I would fix it immediately if I did not have just a couple of edits left before the question gets transferred to the community wiki. Thus I would kindly appreciate if someone with enough rep would fix the two usage examples for me. In any case I will do it myself when I will append my finalized implementation of ObserverPtr to the question. –  jms Mar 22 at 3:19
show 3 more comments

Sounds like what you need is a weak_ptr<>

#include <memory>
#include <iostream>
struct Foo
{
    const char *hello() const
    {
        return "hello world";
    }
};

void test(std::weak_ptr<Foo>& w)
{
    std::cout << (w.expired() ? "expired" : w.lock()->hello()) << "\n";
}

int main()
{
    std::shared_ptr<Foo>    foo(new Foo);
    std::weak_ptr<Foo>      ptrToFoo(foo);

    test(ptrToFoo);
    foo.reset();     // Reset the pointer.
    test(ptrToFoo);
}

Run:

> ./a.out
hello world
expired
share|improve this answer
    
I am aware that I can use a std::weak_ptr to keep track of an object of unknown lifetime (that's the solution I have previously resorted to), but I don't like how I'm then forced to own the object trough a std::shared_ptr and call std::weak_ptr::lock() every time I dereference the pointer. What is the rationale for not providing operator* and operator-> for weak_ptr? I assume that it is related to concurrency and multithreading? –  jms Mar 22 at 7:10
    
If writing my own application specific smart pointer is a bad idea, or just a worse decision than using std::shared_ptr and std::weak_ptr even when ownership semantics and concurrency/multithreading support are not actually needed and only contribute in the form of additional application complexity and lower performance, I would like to know why? –  jms Mar 22 at 7:50
    
Writing your own smart pointer is a very bad idea (only useful as an educational exercise). It is extremely easy to get wrong. Even when you get most of it write the corner cases always catch you. Would you write your own vector class? The provided smart pointers are highly tested and very efficient. –  Loki Astari Mar 22 at 7:57
    
Fine, I will reconsider sticking with the standard smart pointers. In any case I am still going to complete ObserverPtr as an exercise. For me computer programming is just a hobby, so no worries, there is no chance that I am going to infect some commercial codebase with this abomination. –  jms Mar 22 at 8:17
    
@user1062874 If you don't like calling lock and if your code isn't multi-threaded, perhaps you can define a Foo* get(weak_ptr<Foo> weak) { shared_ptr<Foo> shared = weak.lock(); return shared.get(); } function, to get the raw pointer (or null) from a weak_ptr in one function call. –  ChrisW Mar 22 at 16:18
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.