Using 'this' in constructor

[bb]

Hi,

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.

 

[Stephen Horne]

Hi,

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.

Assuming that's significant as opposed to a rushed example mix-up, you
may need to define the constructor separately from the declaration.
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

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.

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.

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 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 ();
};


// 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_;
}

 

[James Kanze]

class A

No.  AImpl is unknown at the point where you're trying to
instantiate it.

Not unknown, just incomplete. (Member function code in a class
is compiled as if it were defined immediately after the class.)
Of course, you can't new an incomplete type, so the code is
still illegal.

If your constructor is defined after 'AImpl' is defined as
well, then yes, it's legal, but keep in mind that the object
is not considered fully constructed until its constructor
completes, so calling member functions can be dangerous,
*generally speaking*.

If *all* your AImpl constructor needs to do is to *store* the
pointer to 'A' somewhere so it can access it later, then yes,
it's going to be OK.

Whether A is complete isn't the problem. Something like:

class A {
public:
A() : p( new AImpl( this ) ) {}

private:
struct AImpl
{
AImpl( A* p ) ;
// ...
} ;
AImpl* p ;
} ;

is legal. (Of course, the names in the example suggest the
compliation firewall idiom, and this would defeat the purpose of
it.)

 

[James Kanze]

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.)