Re: Using 'this' in constructor
On Oct 18, 3:05 am, Stephen Horne <sh006d3...@blueyonder.co.uk> wrote:
On Fri, 17 Oct 2008 09:59:15 -0700 (PDT), bb <muralibal...@gmail.com>
wrote:
Is it legal and ok to use 'this' in a constructor as follows:
Class A {
public:
A() : pimpl_(0) {
pimpl_ = new AImpl(this);
}
private:
struct AImpl;
AImpl* pimpl_;
};
Thanks.
I see two issues. One is the order of the code, as Victor
pointed out. The other is that you haven't defined AImpl -
only declared it.
There's no problem with the order of the code which is there.
The problem is, as you say, the fact that the definition of
AImpl is missing. If he puts a definition of AImpl in class A
(anywhere in class A), and that definition has an accessible
constructor which can be called with an A*, the code is fine.
Assuming that's significant as opposed to a rushed example
mix-up, you may need to define the constructor separately from
the declaration.
This looks like the compilation firewall idiom. If so, for it
to work, AImpl and the constructor must be defined in a separate
source file.
For example, you might have...
// In header
class A
{
private:
struct AImpl;
AImpl* pimpl_;
// should probably use std::auto_ptr<A_impl>
// doing so would avoid the need for the destructor
You can't, legally. (And in fact it doesn't generally work.)
Same problem as above, more or less. std::auto_ptr requires
that the instantiation type be complete.
public:
A ();
virtual ~A();
};
// In cpp file
struct A::AImpl
{
A& owner_ref;
AImpl (A* p_owner_ref) : owner_ref (*p_owner_ref);
{
}
};
A::A ()
{
pimpl_ = new AImpl (this);
}
A::~A ()
{
delete pimpl_;
}
I've got the feeling I'm doing something wrong in that but I'm
not sure what - maybe just tired - but the principles about
right.
No. You've got it right.
As Victor also pointed out, this isn't ideal. If the AImpl
constructor does anything with the instance of A (as opposed
to just the pointer) it is probably broken, since the instance
isn't considered constructed at this point.
This is the standard compilation firewall idiom. Normally, the
only thing the constructor of A does is create the
implementation class. Most of the time, however, the
implementation class doesn't need a pointer back to the client
interface, but there are exceptions.
Issues like this lead to the common style rule that constructors
should do the minimum work necessary to set up a self-consistent
state. More substantial initialisation should be done by a separate
method in a separate call.
I've never seen that rule. In fact...
I don't stick to the rule all that well myself, but it is
something to bear in mind. On this principle, one alternative
(not necessarily a preferred one) is...
// In header
class A
{
private:
struct AImpl;
AImpl* pimpl_;
public:
A ();
virtual ~A();
void Initialise ();
};
This is considered very bad practice by everyone I know. (There
are times it's necessary, but that's a different issue.) The
constructor should fully initialize the object. Always.
Sometimes, there will be a private initialize function, called
by several different constructors. But providing a public
initialize function, and requiring all clients to call it after
construction, is very bad practice.
// In cpp file
struct A::AImpl
{
A& owner_ref;
AImpl (A* p_owner_ref) : owner_ref (*p_owner_ref);
{
}
};
A::A ()
{
pimpl_ = 0;
}
void A::Initialise ()
{
if (pimpl_) throw "Oops - double initialised";
pimpl_ = new AImpl (this);
}
A::~A ()
{
if (pimpl_) delete pimpl_;
That's an unnecessary if.
}
This is a very good example of something that should be avoided
at all costs. It violates an important class invariant, that
pimpl_ always points to a valid AImpl object. (The class
invariant is actually stricter: that pimpl_ always points to the
same AImpl object. Logically, pimpl_ should be a reference, and
not a pointer.)
--
James Kanze (GABI Software) email:james.kanze@gmail.com
Conseils en informatique orient=E9e objet/
Beratung in objektorientierter Datenverarbeitung
9 place S=E9mard, 78210 St.-Cyr-l'=C9cole, France, +33 (0)1 30 23 00 34