James Kanze
10/18/2008 6:46:00 AM
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ée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34