Sign in to follow this  

C++ pointers again, what happens, how safe?

This topic is 3723 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

Quick questions about raw pointers and safety...what happens when:
// Example 1.  What happens to the pointer and allocated memory here?
template<typename T>
void function_that_takes_ptr(T *ptr)
{
    ptr->doSomething();
}

function_that_takes_ptr(new thing);

// Example 2.  Is this pointer behaviour 'perfectly' safe?
template <typename T>
class raw_ptr_class
{
    public:
       ~raw_ptr_class()
       {
           delete ptr;
       }

    private:
       T *ptr;
};

void function_uses_vector_of_ptr_classes(const T &arg)
{
    std::vector<raw_ptr_class<T> >vec;
    // Do something with vector and arg.
}

std::string strg;
function_uses_vector_of_ptr_classes(strg);
I know that auto_ptr<>(), boost::<various pointers> are safer, but I find that passing around auto_ptrs etc can get quite troublesome, and raw pointers are sometimes the only choice, so I'm trying to better understand what's going on 'under the hood'. --random

Share this post


Link to post
Share on other sites
When dealing with pointers, it's essential to determine ownership. Who has the responsibility to delete it when it's no longer needed?
In your code, ownership starts out at the call site, but is transferred to the function you call since there's no way to hold on to the pointer at the call site.
Thus, it's now function_that_takes_ptr that owns the pointed to memory, and it has to either give some other entity ownership of it, or clean it up.
Unless doSomething() assumes ownership of the pointer it's invoked with (unlikely), you will have a leak.

The class in your second example has several flaws. It doesn't have a copy constructor, nor assignment operator. It also doesn't initialize the pointer, so upon destruction it will attempt delete an indeterminate memory position.
Note that deleting a null pointer is valid and a no-op, but deleting an uninitialized pointer is UB.

Share this post


Link to post
Share on other sites
Thanks for the info...OK, in the example 2 I assumed that the compiler would automatically create all the copy ctors and assignments, but I get your point about it not being intitialized. How about this modification...


// Example 2. Is this pointer behaviour 'perfectly' safe?
template <typename T>
class raw_ptr_class
{
public:
raw_ptr_class():ptr(new T){ }
~raw_ptr_class()
{
delete ptr;
}

private:
T *ptr;
};

void function_uses_vector_of_ptr_classes(const T &arg)
{
std::vector<raw_ptr_class<T> >vec;
// Do something with vector and arg.
}

std::string strg;
function_uses_vector_of_ptr_classes(strg);


Also, are you saying in Example 1 that when the pointer goes out of scope there will be a potential memory leak, or will the pointer be deleted, as any other variable would be removed from memory when the function goes out of scope? In other words, is it necessary to explicitly delete (and zero) pointers that are passed to functions in this way?

Edit: This is another question -- 'good practice' with pointers advises that the pointer be deleted and set to zero, no? So:


T *ptr;
ptr = new T;

// ...do something...

delete ptr;
ptr = 0;


But in the case of the destructor of a class, I've been taught that only delete is required, and resetting pointers is not required. Why?

--random


[Edited by - random_thinker on October 3, 2007 6:40:13 AM]

Share this post


Link to post
Share on other sites
there is no reason to set a deleted pointer to 0. It will only give you a false sense of security. Your new code is much better but still has errors. Your raw pointer class needs copy/assignment operators or it is not safe. You see look at this example:

raw_ptr_class<int> a;
raw_ptr_class<int> b;
a=b;

You have a memory leak AND a double deletion. You see "a" creates and object and assigns it to a.ptr, b does the same for b.ptr. Then when you assign b to a then both ptr's point to the object created by b. Who is pointing to the object created by a? No one. That is a memory leak. Then it comes time for the destructors. b's destructor gets called and everything is fine. However when a tries to delete the object that it shared with b, well things are not fine and your program crashes

Share this post


Link to post
Share on other sites
When all pointers to an object with dynamic storage go out of scope, the object is lost forever, thus creating a memory leak.
Local variables have automatic storage, and are cleaned up when their scope is exited.
Objects with dynamic storage should be deleted once. That is why determining ownership is important, so that the object is not deleted zero, two or more times, which is an error.

As for assigning null to deleted pointers, it's mostly a matter of preference.
Some argue that it helps avoid using invalid pointers, but one shouldn't use pointers not pointing at anything at all.
There may also be more than one pointer to the object, and you would then have to null all out to be consistent.

Share this post


Link to post
Share on other sites
Rule of three, if you need any of destructor, overloaded assignment operator or copy constructor, then you probably need all of them. If you wish, you can make operator=() and the copy constructor private (and unimplemented).

Your raw_ptr_class is not safe for use in a std::vector because it doesn't obey proper copy construction.

Share this post


Link to post
Share on other sites
Thanks for the information, it's helping greatly. But coming back to the issue of class member pointers, copy ctors and assignment operators, I was not aware of such a problem, thank you for educating me on this.

I am surprised that the pointer does not need to be reset to zero (or NULL). So many websites/books recommend this approach.

Also, I use this technique with composition and pimpl type classes. So what you are telling me now, is that there is likely a gaping hole in these classes, and that I either need to declare copy ctors and assignment operators private (use boost::noncopyable) or go with an auto_ptr type technique.

--random

Share this post


Link to post
Share on other sites
Just to summarize, to make sure that I understand:

1. Whoever owns the pointer, is responsible for deleting it when it is no longer needed.
2. Classes with raw pointers must carefully control creation, copying, assignment and deletion.
3. When deleted, there is no need to reset to zero or NULL.
4. Use std::auto_ptr<> or boost::<various pointers> whenever possible.

Do you think that would solve the majority of pointer-related memory leaks?

--random

Share this post


Link to post
Share on other sites
Quote:
Original post by random_thinker
Just to summarize, to make sure that I understand:

1. Whoever owns the pointer, is responsible for deleting it when it is no longer needed.
2. Classes with raw pointers must carefully control creation, copying, assignment and deletion.
3. When deleted, there is no need to reset to zero or NULL.
4. Use std::auto_ptr<> or boost::<various pointers> whenever possible.

Do you think that would solve the majority of pointer-related memory leaks?

--random


It isn't perfect. A problem with boost::shared_ptr this is circular references. One must be extremely careful with std::auto_ptr as it has unusual (and arguably unintuitive) copy semantics.

When you reset a pointer, only set the pointer to NULL if it is to be used again. A simple example might be a lemmings game, you could keep a pointer (or pointer object like boost::shared_ptr/weak_ptr) to the currently selected lemming somewhere, or a NULL pointer if no lemming is currently active.

If the object isn't intended to be used again there is a possibility, as mentioned, that logically incorrect double deletes will go undetected.

Share this post


Link to post
Share on other sites
Quote:
Original post by random_thinker
Thanks for the information, it's helping greatly. But coming back to the issue of class member pointers, copy ctors and assignment operators, I was not aware of such a problem, thank you for educating me on this.

I am surprised that the pointer does not need to be reset to zero (or NULL). So many websites/books recommend this approach.

Also, I use this technique with composition and pimpl type classes. So what you are telling me now, is that there is likely a gaping hole in these classes, and that I either need to declare copy ctors and assignment operators private (use boost::noncopyable) or go with an auto_ptr type technique.

--random


Polymorphic pImpl-pointer (own-a-copy semantics) with virtual clone idiom:


struct Impl {
virtual Impl* clone() = 0;
virtual void act() = 0;
};

struct FooImpl {
virtual void act() { /* do something */ }
virtual FooImpl* clone() { return new FooImpl(this); }
};

struct BarImpl {
virtual void act() { /* do something else */ }
virtual BarImpl* clone() { return new BarImpl(this); }
};

class Wrapper {
Impl* impl;

public:
void act() { impl-&gt;act(); }

Wrapper(Impl* impl): impl(impl) {}
~Wrapper() { delete impl; }
Wrapper(const Wrapper& other): impl(other.impl-&gt;clone()) {}
Wrapper& operator=(const Wrapper& rhs) {
Wrapper other(rhs);
std::swap(impl, other.impl);
return *this;
}
};

Share this post


Link to post
Share on other sites
Quote:
Original post by random_thinker
Just to summarize, to make sure that I understand:

1. Whoever owns the pointer, is responsible for deleting it when it is no longer needed.
2. Classes with raw pointers must carefully control creation, copying, assignment and deletion.
3. When deleted, there is no need to reset to zero or NULL.
4. Use std::auto_ptr<> or boost::<various pointers> whenever possible.

Do you think that would solve the majority of pointer-related memory leaks?

--random


1) Right.
2) Very much so. To the extent that at my job our code review process no longer allows raw pointers except where library calls demand them.
3) NULL any pointer that you delete if the pointer variable isn't about to go out of scope. It's a simple if(!pWhatever) test to check for NULL, and delete NULL is legal, but you can't easily test for deleted memory. Code grows, so this is an important guard against future errors.
4) I find auto_ptr's semantics to be very rarely useful, and occasionally dangerous. Just about anything I can use auto_ptr for, I can use a stack allocation better. boost::shared_ptr and boost::weak_ptr are much better designed smart pointers.

- V

Share this post


Link to post
Share on other sites
Quote:

Polymorphic pImpl-pointer (own-a-copy semantics) with virtual clone idiom:


Thx Zahlman, I'm going to have to study that for a while to understand exactly what it is doing. I've gone through the code below with some comments...help?


// Pure virtual base struct, two methods (1) clone a pointer, (2) create actions..
struct Impl
{
virtual Impl* clone() = 0;
virtual void act() = 0;
};

// Shouldn't FooImpl inherit Impl?
struct FooImpl : public Impl
{
virtual void act() { /* do something */ }
virtual FooImpl* clone() { return new FooImpl(this); }
};

// Shouldn't BarImpl inherit Impl?
struct BarImpl : public Impl
{
virtual void act() { /* do something else */ }
virtual BarImpl* clone() { return new BarImpl(this); }
};

// Wrapper for different types of Impl's...
class Wrapper
{
// Pointer of ABC type Impl...
Impl* impl;

public:
// Calls action from underlying pointer...
void act() { impl->act(); }

// Constructor...uses ABC type Impl
Wrapper(Impl* impl): impl(impl) {}

// Destructor...
~Wrapper() { delete impl; }

// Copy constructor...uses underlying clone method...
Wrapper(const Wrapper& other): impl(other.impl->clone()) {}

// Assignment operator...uses copy ctor to clone rhs then swap values...
Wrapper& operator=(const Wrapper& rhs)
{
Wrapper other(rhs); // Uses copy constructor...
std::swap(impl, other.impl); // swaps values...
return *this; // returns swapped value...
}
};

// Usage...initialize...
Wrapper ptr1(new FooImpl),ptr2(new BarImpl);
ptr1 = ptr2; // Now ptr1 is a BarImpl and ptr2 is a FooImpl....
Wrapper ptr3(ptr1); // Now ptr3 is a BarImpl...
ptr1->act(); // Calls BarImpl action...
ptr2->act(); // Calls FooImpl action...
ptr3->act(); // Calls BarImpl action...




Have I got this right or is there more...shouldn't Foo... and Bar... inherit Impl? What about virtual destructors in Impl and Foo.../Bar...? Are they necessary? Are you suggesting that this could be a wrapper for all pointers to minimize/eliminate risk of memory leaks? What would be a typical use for act()?

Questions, questions...

--random

Share this post


Link to post
Share on other sites
Quote:
Original post by random_thinker
Quote:

Polymorphic pImpl-pointer (own-a-copy semantics) with virtual clone idiom:


Thx Zahlman, I'm going to have to study that for a while to understand exactly what it is doing. I've gone through the code below with some comments...help?

*** Source Snippet Removed ***

Have I got this right or is there more...shouldn't Foo... and Bar... inherit Impl? What about virtual destructors in Impl and Foo.../Bar...? Are they necessary? Are you suggesting that this could be a wrapper for all pointers to minimize/eliminate risk of memory leaks? What would be a typical use for act()?

Questions, questions...

--random


Need a virtual destructor in the base class, you could make it pure virtual (
virtual ~T() = 0 {}
), and then the derived implementations would be required to implement it.

Share this post


Link to post
Share on other sites
Quote:
Original post by random_thinker
Have I got this right or is there more...shouldn't Foo... and Bar... inherit Impl?


They should. I've been doing too much Python ;)

Quote:
What about virtual destructors in Impl and Foo.../Bar...? Are they necessary?


Yes. Rather, like Washu said. Again, complete oversight on my part.

Though it seems to me that it's sufficient to have virtual ~Impl() {} in the base, and only implement destructors for derived classes where they're needed. You should get the generated derived dtor being called (because the base dtor is usual), then the base dtor on the base part of the object (following the usual rules), and everything will be OK. I'm not a big fan of requiring users to implement things that will usual be trivial in the interest of "safety" - real safety comes from proper design which sidesteps the need for these things.

Quote:
Are you suggesting that this could be a wrapper for all pointers to minimize/eliminate risk of memory leaks?


No; only for pointers that are used in this way. There are certainly reasons to have multiple pointers pointing at a common object, and this doesn't handle those cases (it treats the pointer as a proxy for the common object, copying it whenever the pointer would be copied). You should still make use of things like boost::shared_ptr where appropriate.

That said, it's entirely possible for a meaningful project not to require any smart pointers beyond this type. :) It depends on the problem domain.

Quote:
What would be a typical use for act()?


It's just a sample of any "normal" member function of the wrapper class. You just delegate everything to the implementation, and calling code can use the wrapper just as if it were a non-polymorphic type (i.e. making copies, declaring variables of the type rather than pointers thereto) except that it behaves polymorphically.

Note that this approach "reuses" the pointer-indirection overhead that you need anyway for the pImpl idiom in order to get the polymorphic behaviour as well. You're still going through a wrapper function call (though I would hope compilers can inline this particular case fairly well now), pointer indirection to the implementation, and then dynamic dispatch for the virtual logic - but that shouldn't be an excuse to try to beat the compiler. ;)

To complete the illusion, it's usual to hide the implementation classes from the rest of the code completely, make the Wrapper(Impl*) constructor private, and provide some kind of factory interface instead. (It can be as simple as offering normal constructors for the Wrapper, which contain logic to select and allocate an implementation object.)

Share this post


Link to post
Share on other sites

This topic is 3723 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.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this