Sign in to follow this  
scottdewald

auto_ptr within Pimpl Class

Recommended Posts

scottdewald    163
I read in another forum the other day that you should not use the std::auto_ptr class to hold the member data pointer within a class that follows the Pimpl Idiom class. The person said that auto_ptr required a complete class. He said that Herb Sutter's implementation was wrong. My compiler (gcc under cygwin) allows it as long as I put my class constructor within the source file. Can you use the auto_ptr class if you overload the copy constructor and assignment operator? Was this guy wrong? Thanks, Scott

Share this post


Link to post
Share on other sites
DaBono    1496
Quote:
My compiler (gcc under cygwin) allows it as long as I put my class constructor within the source file.
Which makes sense, since the class is completely declared there.

Apart from the question if it is accepted or not, I wonder why you would want to use an auto_ptr to implement the Pimpl-idiom. Your interface class will and should be the only one referencing this pointer, and would always keep explicit ownership.

Share this post


Link to post
Share on other sites
scottdewald    163
Quote:
Original post by DaBono
... Your interface class will and should be the only one referencing this pointer, and would always keep explicit ownership.
Exactly. However, using the auto_ptr class as Herb Sutter suggested removes any possibility of forgetting to free the memory in the destructor.

Share this post


Link to post
Share on other sites
ToohrVyk    1596
Quote:
Original post by DaBono
Apart from the question if it is accepted or not, I wonder why you would want to use an auto_ptr to implement the Pimpl-idiom. Your interface class will and should be the only one referencing this pointer, and would always keep explicit ownership.


This saves the pain of writing a destructor (since the default destructor would destruct the auto_ptr, and thus the implementation pointer.

Share this post


Link to post
Share on other sites
scottdewald    163
Quote:
Original post by ToohrVyk
This saves the pain of writing a destructor (since the default destructor would destruct the auto_ptr, and thus the implementation pointer.

So, I assume you're confirming that it is OK by the standard to implement the Pimpl idiom using an auto_ptr?

Share this post


Link to post
Share on other sites
Bregma    9214
Quote:
Original post by scottdewald
Was this guy wrong?


Yes and no. std::auto_ptr requires that the destructor of the class be available. That should the case in the imeplentation file for the class your are wrapping using the pimpl idiom.

I would recommend using std::auto_ptr in your pimpl class. It does the right thing at the right time and documents pointer ownership. It forces you to write appropriate copy constructors and assignment operators. It helps you avoid common errors.

--smw

Share this post


Link to post
Share on other sites
rip-off    10979
I think the issue with the pimpl idiom and auto_ptr is about the destructor of the pimpl class.

Eg if the pimpl class was declared with a virtual destructor / custom operator delete etc.

For example, the following code gives this error under gcc:

class PimplUser
{
public:
PimplUser();
private:
class Pimpl;
std::auto_ptr<Pimpl> pimpl;
};

void foo()
{
PimplUser pimplUser;
}

// C:\Dev-Cpp\include\c++\3.4.2\memory In destructor `std::auto_ptr<_Tp>::~auto_ptr() [with _Tp = PimplUser::Pimpl]':
// 473 D:\Programming\c++\rip_asteroids\src\common\main.cpp instantiated from here
// 258 C:\Dev-Cpp\include\c++\3.4.2\memory [Warning] possible problem detected in invocation of delete operator:
// 258 C:\Dev-Cpp\include\c++\3.4.2\memory [Warning] invalid use of undefined type `struct PimplUser::Pimpl'
// 467 D:\Programming\c++\rip_asteroids\src\common\main.cpp [Warning] forward declaration of `struct PimplUser::Pimpl'
// 467 D:\Programming\c++\rip_asteroids\src\common\main.cpp neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined.




[edit:] Yeah, what Bregma said. Curse leaving the computer and finishing posts wihtout checking for an update [smile]

Share this post


Link to post
Share on other sites
Zahlman    1682
Quote:
Original post by Bregma
I would recommend using std::auto_ptr in your pimpl class. It does the right thing at the right time and documents pointer ownership. It forces you to write appropriate copy constructors and assignment operators.


?!

std::auto_ptr provides a copy constructor and assignment operator, so it doesn't force you to write any (the pimpl's auto-generated cctor and assignop would just delegate to the auto_ptr, in effect). Meanwhile, the auto_ptr's semantics for those arguments are, generally speaking, WRONG for a pimpl; the original object will get nuked.

... Oh, wait. It *will* force writing those operations, won't it? Because of const-correctness issues (auto_ptr's assignop and cctor accept non-const references, so they won't be liked by the default-generator)?

But then, that just means more work - you could instead implement a auto-ptr-alike that implements the proper semantics (i.e., invoking the pointed-at-thing's cctor and assignop), and never have to put that code into any pimpl you create. Not to mention, *nothing* you put into the pimpl can force you to write a proper cctor or assignop for *the worker class*. :s Unless I'm just spectacularly unimaginative?

Share this post


Link to post
Share on other sites
scottdewald    163
I ran a few tests with a class that had an auto_ptr as a member variable, and my compiler (gcc under cygwin) required me to overload operator= to deal with the const correctness issue, but it did *not* require me to overload the copy constructor. This surprised me since the auto_ptr class doesn't have the appropriate const copy constructor for this. Am I missing something or is my compiler wrong?

Share this post


Link to post
Share on other sites
Quote:
Original post by ToohrVyk
Quote:
Original post by DaBono
Apart from the question if it is accepted or not, I wonder why you would want to use an auto_ptr to implement the Pimpl-idiom. Your interface class will and should be the only one referencing this pointer, and would always keep explicit ownership.


This saves the pain of writing a destructor (since the default destructor would destruct the auto_ptr, and thus the implementation pointer.


Yes and no. If you let the compiler write the destructor for you, you have to make sure that the encapsulated type is complete - otherwise your encapsulated object will not be destroyed (in most implementation).

The following code leaks:

// A.h
class A
{
std::vector<int> v;
public:
A(std::size_t s) : v(s) { }
};

// B.h
class A; // forward declaration

class B
{
std::auto_ptr<A> a;
public:
B();
};

// B.cpp
#include "B.h"
#include "A.h"

B::B() : a(new A(100)) { }
;

The reason is that the implicit destructor of B will call the destructor of auto_ptr<A>; since A is not a complete type, the compiler will either issue a warning or will silently give A an implicit trivial destructor (and as a consequence, this destructor will not release the allocated vector).

To correct the problem, simply add a destructor to class B, and put the destructor code in B.cpp (even if the destructor does nothing).

Regards,

Share this post


Link to post
Share on other sites

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