Using this pointer in constructor

I've read before that using the this pointer in a constructor should be avoided. Generally I can understand that the object may not be entirely constructed yet, but I've never understood entirely what the potential issues are.

This SO thread mentions:

"'this' should not be used in a constructor anyways unless you know what you're doing and don't mind the limitations"

(They also deal with shared pointers, which I should be doing...)

This cboard thread doesn't really have any bad things to say about the practice.

In some test code, I didn't as much as get a warning from the compiler:

Code:

// thistest.cpp

class A;

class B {
public:
B(A *a);
private:
A *a_;
};

class A {
public:
A();
private:
B b_;
};

A::A()
: b_(this)
{ }

B::B(A *a)
: a_(a)
{ }

g++ version 4.6.3
g++ -Wall -c thistest.cpp

For something more concrete, I have a Model class, which is more or less a fancy symbol table. I don't want to give all clients full access to the model, so I created another class, ModelReader. ModelReader can return a requested symbol from the Model, but can't do much else. I would like it if a Model could provide its own ModelReader.

Code:

#include "modelreader.h"

class Model {
public:
Model();
ModelReader *reader() const;
//And some other stuff
private:
ModelReader reader_;
};

Model::Model() {
: reader_(this)
{ }

ModelReader *Model::reader() const
{
return reader_;
}

Code:

// modelreader.h

class Model;

class ModelReader {
public:
ModelReader(Model *model);
// Some other functions here to make it useful
private:
Model *model_;
};

ModelReader::ModelReader(Model *model)
: model_(model)
{ }

Are there any limitations to this approach?
Are there better design alternatives?
In general, what are the limitations / caveats to using the this pointer in a constructor?

Thanks in advance

I see that you return a pointer in some public function (s). I would recommend not. I would recommend you to use & instead of *. Also mind that this way you give access to the user (!). Use at least a const reference on return

If you're returning a pointer or reference to class internals, you're giving full access to those class internals.


On the question of using this in a constructor: it is acceptable to use it, but with care.

The thing to watch, as with any constructor, is that the object is not fully constructed before the constructor completes. If you attempt to use it as if it points to a fully constructed object, then the results are either something you won't expect (virtual function dispatch doesn't call the function corresponding to the actual type of object being constructed) or undefined behaviour (eg accessing the value of a class member before the constructor has initialised it).

It is perfectly safe to store the value of the this pointer (for example, in an array) as the value of this is determined before the constructor is invoked. The potential dangers are in using it before the constructor completes.

Agreed, this is a perfectly acceptable use of the this pointer.

Are there any limitations to this approach?
Are there better design alternatives?
In general, what are the limitations / caveats to using the this pointer in a constructor?

When it comes to your example, what you are doing is very dangerous and you should definately change your approach. You pass the this pointer of a not-yet-constructed Reader (not all members have been initialized yet) to a different class. If that class (ModelReader) attempts to call any member function of the Reader it recieves (in its constructor), the behaviour is undefined.

And these are not the only issues with the this pointer.

I don't know your exact goals, but what you should definately do is to remove the "ModelReader reader_;" member from Model. Why not allow users construct their own instances of ModelReader?

Originally Posted by kmdv

When it comes to your example, what you are doing is very dangerous and you should definately change your approach. You pass the this pointer of a not-yet-constructed Reader (not all members have been initialized yet) to a different class. If that class (ModelReader) attempts to call any member function of the Reader it recieves (in its constructor), the behaviour is undefined.

And these are not the only issues with the this pointer.

I don't know your exact goals, but what you should definately do is to remove the "ModelReader reader_;" member from Model. Why not allow users construct their own instances of ModelReader?

*Sigh*. Obviously you've never encountered this before. It is a well known problem (a sub-object having to know about its outer-object) with a well used solution (pass in the this pointer to the sub-object). Grumpy's post already covered that it would be bad if accessing the item through the this pointer in the constructor, but that should not be going to happen here.

It's fine jason_m. There's no need to change anything as you're already using the common solution to the problem.

Originally Posted by iMalc

It is a well known problem (a sub-object having to know about its outer-object) with a well used solution (pass in the this pointer to the sub-object).

What he is doing is "sub-object knowing about its uninitialised outer-object" which is quite a specific case of simple "sub-object knowing about its outer-object". Storing pointer acquired during member initialisation is not always safe as well, e.g., an accidental upcast to an uninitialised base may happen, which results in undefined behaviour. Obviously, this is not the case in the simple example he posted.
I do not understand his idea of having ModelReader instance being "fixed" in Model, but I am not getting into detail because there is not enough information. However, whether he wants to pass the this pointer to the reader only, or to the other subobjects as well (his "symbol table"), what I would recommend is to put such initialisation outside of the member initialisation list if possible - this at least guarantees no undefined behaviour.

Originally Posted by kmdv

What he is doing is "sub-object knowing about its uninitialised outer-object" which is quite a specific case of simple "sub-object knowing about its outer-object".

That's why, in my previous post, I had a rider about not using (i.e. dereferencing) the pointer until the constructor is complete.

Originally Posted by kmdv

Storing pointer acquired during member initialisation is not always safe as well, e.g., an accidental upcast to an uninitialised base may happen, which results in undefined behaviour.

Not true.

Firstly, within a constructor, "this" always has a static type of (const, depending on age of compiler) pointer to the type being initialised. Second, there are no circumstances in which converting a pointer to derived into a pointer to base yields undefined behaviour (the result is either ambiguity, in which case the code won't compile, or a successful conversion). Third, base class constructors are always invoked before derived class constructors so, if such an "accidental upcast" occurs, dereferencing that converted pointer always has a well defined result.

The only way to introduce undefined behaviour is to deliberately subvert the type system (e.g. use an explicit conversion to bludgeon the compiler into submission when it complains about an invalid implicit conversion) and THEN to dereference the resultant pointer. Since such a conversion is quite deliberate, it would not qualify as an "accidental upcast".

Thanks for the feedback. My specific take away from this is it is fine to store the pointer; it is not okay to dereference it or try to access any members through it.

So I've re-worked the example to see if I could do something "bad":

Code:

// modeltest.cpp
#include <iostream>
#include <string>

class Model;

class ModelReader {
public:
    ModelReader(Model *m);

private:
    Model *model_;
};

class Model {
public:
    Model();
    std::string getString() const;

private:
    ModelReader reader_;
    std::string *str_;
};

Model::Model() 
    : reader_(this), str_(new std::string("this might not work"))
{ 
    // In the initializer list, reader_ is constructed, and reader_'s
    // constructor calls Model::getString before the model's
    // str_ member is constructed
}

std::string Model::getString() const
{ return *str_; } // hasn't been bound to an object prior to construting reader_

ModelReader::ModelReader(Model *m)
    : model_(m)
{
    std::cout << m->getString() << std::endl;
}

int main()
{
    Model m;
    std::cout << "Still alive" << std::endl;
    return 0;
}

Sure enough, this resulted in a seg fault.

ModelReader does not attempt to access any members of the Model passed in its constructor. It only holds on to the pointer for future use. Although I can see how this could be dangerous. Maybe a month or two from now I want to make a quick change to ModelReader and forget that I can't access members of the Model. I think this is the point that kmdv is making.

I'll also go off the topic a little and address some of the other comments you've made about the approach in general.

Originally Posted by std10093

I see that you return a pointer in some public function (s). I would recommend not. I would recommend you to use & instead of *. Also mind that this way you give access to the user (!). Use at least a const reference on return

Originally Posted by grumpy

If you're returning a pointer or reference to class internals, you're giving full access to those class internals.

I'm not sure I know what this means. Are you saying that by returning the address of the ModelReader, someone could compute memory address offsets and access other class members I wanted to hide? For example, someone could compute where the pointer back to the Model lives, and then get at the very thing I am trying to hide with the ModelReader.

Whether the function returns a pointer or a reference, I think this would be possible (just apply the address-of operator to the reference).

But even still, if I passed a copy object, it would still have a pointer back to the Model in it. Can't I still grab the Model pointer out of there if I really wanted to?

My lazy answer to this question is that I program by hobby, primarily for myself, so I usually don't think about what other malicious programmers might do with my code. A slightly more thoughtful answer is that unless you completely hide the implementation with something like the PIMPL idiom, couldn't another programmer always get at class internals if they wanted to bad enough?

Originally Posted by kmdv

I don't know your exact goals, but what you should definately do is to remove the "ModelReader reader_;" member from Model. Why not allow users construct their own instances of ModelReader?

Every symbol that lives in the model gets a "read-only" view of the Model through a ModelReader. That way, symbols can check the values of other symbols that they depend on, and recalculate if necessary when something in the Model changes. But I don't want symbols to be able to create/remove/edit/iterate over the symbols in the Model. That is why I created the ModelReader in the first place. Since every symbol will use a ModelReader, it felt wasteful to me to construct one for every symbol. So I put that responsibility on the Model itself, and then when a symbol registers itself with the Model, the Model passes a ModelReader to the symbol. Only one ModelReader every needs to be constructed.

Originally Posted by jason_m

ModelReader does not attempt to access any members of the Model passed in its constructor. It only holds on to the pointer for future use. Although I can see how this could be dangerous. Maybe a month or two from now I want to make a quick change to ModelReader and forget that I can't access members of the Model. I think this is the point that kmdv is making.

Possibly. My point (which amounts to "don't do that") is not in conflict with that conclusion.

Originally Posted by jason_m

I'm not sure I know what this means.

Give me a pointer to a ModelReader, I can do anything I like to the ModelReader which is pointed at, regardless of whether it is owned by your Model class or not.

Originally Posted by jason_m

Are you saying that by returning the address of the ModelReader, someone could compute memory address offsets and access other class members I wanted to hide? For example, someone could compute where the pointer back to the Model lives, and then get at the very thing I am trying to hide with the ModelReader.

Technically speaking, yes, someone could compete memory offsets and access other class members you want to hide. Access control (public, private, protected) is designed to prevent accidental or unwanted access, not to prevent deliberate access if someone is determined enough.

All you're really doing, however, is what security folks describe as "security through obfuscation", which is among of the weakest means of achieving security against a deliberate attempt to access. There is nothing technically preventing access, if someone is determined enough to reverse-engineer your obfuscation scheme.

If security is your goal, then you need to be clear about what type of security you intend. If your intent is to prevent deliberate attempts to access your model data, then your approach is pointless, since it doesn't achieve that. If your intent is to prevent accidental access of your model data, then your approach is relatively weak, as it is easy to accidentally subvert.

Originally Posted by grumpy

Not true.

Firstly, within a constructor, "this" always has a static type of (const, depending on age of compiler) pointer to the type being initialised.

I am talking about standard C++, not about ancient compilers.

Originally Posted by grumpy

Second, there are no circumstances in which converting a pointer to derived into a pointer to base yields undefined behaviour (the result is either ambiguity, in which case the code won't compile, or a successful conversion). Third, base class constructors are always invoked before derived class constructors so, if such an "accidental upcast" occurs, dereferencing that converted pointer always has a well defined result.

The only way to introduce undefined behaviour is to deliberately subvert the type system (e.g. use an explicit conversion to bludgeon the compiler into submission when it complains about an invalid implicit conversion) and THEN to dereference the resultant pointer. Since such a conversion is quite deliberate, it would not qualify as an "accidental upcast".

An accidental upcast may happen without subverting the type system at all, e.g., by passing the this pointer to the constructor of one of the base classes.

In 3.8.5 we read:

Before the lifetime of an object has started but after the storage which the object will occupy has been
allocated38 or, after the lifetime of an object has ended and before the storage which the object occupied is
reused or released, any pointer that refers to the storage location where the object will be or was located
may be used but only in limited ways. For an object under construction or destruction, see 12.7. Otherwise,
such a pointer refers to allocated storage (3.7.4.2), and using the pointer as if the pointer were of type void*,
is well-defined. Such a pointer may be dereferenced but the resulting lvalue may only be used in limited
ways, as described below. The program has undefined behavior if:
...
the pointer is implicitly converted (4.10) to a pointer to a base class type
...

See examples in 12.7.1 and 12.7.3.

Originally Posted by jason_m

Sure enough, this resulted in a seg fault.

ModelReader does not attempt to access any members of the Model passed in its constructor. It only holds on to the pointer for future use.

Huh?!, it is accessing members of the Model passed in the constructor, which is precisely why it is crashing!...

Line 26 constructs reader_ which is of type ModelReader, thus calling the ModelReader constructor, and passing it the partially initialised Model. At line 36, 'm' is now the pointer to the partially initialised Model. You then call getString on that instance. getString tries to copy-construct a new string upon its return, and this explodes.

In this instance it could be fixed by switching lines 21 and 22 around so that they are initialised in the other order. (Constructor initialisation lists initialise things in the order they are declared). However that is a poor solution. The ModelReader class should simply not do anything with the pointer passed to it other than to store it for future use, as was discussed.

Originally Posted by kmdv

I am talking about standard C++, not about ancient compilers.

So was I. My qualification about age of compiler was to capture the fact that it also applied to pre-standard C++.

Originally Posted by km17dv

In 3.8.5 we read:

In any event, the words "Before the lifetime of an object has started" at the start of the quote are relevant here, as they describe what happens before the constructor is invoked.

You also, IIRC, substituted words "base class type" for the words "virtual base class" in the text you highlighted in bold, although the distinction is not relevant here.

Originally Posted by km17dv

See examples in 12.7.1 and 12.7.3.

Those examples describe referring to a non-static member or base class of an object, either before constructors begin execution (12.7.1) or after the destructor completes execution (12.7.3).

We are discussing actions by a constructor which (by definition) cannot occur before the constructor begins execution, and also cannot occur before the destructor completes execution.

Originally Posted by jason_m

ModelReader does not attempt to access any members of the Model passed in its constructor. It only holds on to the pointer for future use. Although I can see how this could be dangerous. Maybe a month or two from now I want to make a quick change to ModelReader and forget that I can't access members of the Model. I think this is the point that kmdv is making.

My point was that if you can eliminate code that limits you to do something, why not do that? Of course, only if that is possible and does not make any other harm. In the future, you might want to call some member functions from ModelReader when it recieves a pointer to Model, either via constructor or a member function.

Originally Posted by jason_m

Every symbol that lives in the model gets a "read-only" view of the Model through a ModelReader. That way, symbols can check the values of other symbols that they depend on, and recalculate if necessary when something in the Model changes. But I don't want symbols to be able to create/remove/edit/iterate over the symbols in the Model. That is why I created the ModelReader in the first place. Since every symbol will use a ModelReader, it felt wasteful to me to construct one for every symbol. So I put that responsibility on the Model itself, and then when a symbol registers itself with the Model, the Model passes a ModelReader to the symbol. Only one ModelReader every needs to be constructed.

It seems to be perfect to make use of const-correctness and pass `const Model*` to every symbol. And why not make symbols able to (read-only) iterate over the symbols too? It is pretty to rare to limit const-correctness in this way.

Originally Posted by grumpy

In any event, the words "Before the lifetime of an object has started" at the start of the quote are relevant here, as they describe what happens before the constructor is invoked.

No, in this case they describe what happens before the constructor finishes. Again... from §3.8:

The lifetime of an object is a runtime property of the object. An object is said to have non-trivial initialization
if it is of a class or aggregate type and it or one of its members is initialized by a constructor other than a trivial
default constructor. [ Note: initialization by a trivial copy/move constructor is non-trivial initialization. —
end note ] The lifetime of an object of type T begins when:
— storage with the proper alignment and size for type T is obtained, and
— if the object has non-trivial initialization, its initialization is complete.

You could look up such things in the standard before posting through.

Originally Posted by grumpy

Those examples describe referring to a non-static member or base class of an object, either before constructors begin execution (12.7.1) or after the destructor completes execution (12.7.3).

We are discussing actions by a constructor which (by definition) cannot occur before the constructor begins execution, and also cannot occur before the destructor completes execution.

These examples were given to you only to show you that undefined behaviour by accidental upcast is possible.

By saying before that "storing pointer is not always safe [...]" I did not have every possible situation in mind... just threw a possibility out of my head .

Given constructor phases:
- initialisation of base classes,
- member initialisation,
- execution of constructor's body.

My whole point was to move initialisation of ModelReader (or any other sub-object requiring access to Model) from the phase 2 to the phase 3 - if possible of course. Then you can safely call member functions of Model without risking that some members are not yet constructed. Alternatively OP could ensure that ModelReader is the last one being initialised.

Originally Posted by jason_m

Sure enough, this resulted in a seg fault.

ModelReader does not attempt to access any members of the Model passed in its constructor.

Originally Posted by iMalc

Huh?!, it is accessing members of the Model passed in the constructor, which is precisely why it is crashing!...

Sorry, I wasn't very clear. My example resulted in a seg fault, that was the intention - to access a member of the Model before it was finished being constructed.

The next paragraph was about my actual design, not the little example I put together. In my actual design, the the Model pointer is only stored. Model members are not being accessed in the ModelReader.