Like Tree1Likes

Using this pointer in constructor

This is a discussion on Using this pointer in constructor within the C++ Programming forums, part of the General Programming Boards category; I've read before that using the this pointer in a constructor should be avoided. Generally I can understand that the ...

  1. #1
    Registered User
    Join Date
    May 2008
    Posts
    87

    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

  2. #2
    SAMARAS std10093's Avatar
    Join Date
    Jan 2011
    Location
    Nice, France
    Posts
    2,694
    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
    Code - functions and small libraries I use


    It’s 2014 and I still use printf() for debugging.


    "Programs must be written for people to read, and only incidentally for machines to execute. " —Harold Abelson

  3. #3
    Registered User
    Join Date
    Jun 2005
    Posts
    6,537
    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.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy in reply to you, it is likely you deserve it. Suck it up, sunshine, and read this, this, and this before posting again.

  4. #4
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    Agreed, this is a perfectly acceptable use of the this pointer.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  5. #5
    Registered User
    Join Date
    Aug 2010
    Location
    Poland
    Posts
    682
    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?
    I never put signature, but I decided to make an exception.

  6. #6
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    Quote Originally Posted by kmdv View Post
    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.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  7. #7
    Registered User
    Join Date
    Aug 2010
    Location
    Poland
    Posts
    682
    Quote Originally Posted by iMalc View Post
    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.
    Last edited by kmdv; 12-30-2012 at 06:10 PM.
    I never put signature, but I decided to make an exception.

  8. #8
    Registered User
    Join Date
    Jun 2005
    Posts
    6,537
    Quote Originally Posted by kmdv View Post
    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.

    Quote Originally Posted by kmdv View Post
    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".
    iMalc likes this.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy in reply to you, it is likely you deserve it. Suck it up, sunshine, and read this, this, and this before posting again.

  9. #9
    Registered User
    Join Date
    May 2008
    Posts
    87
    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.

    Quote Originally Posted by std10093 View Post
    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
    Quote Originally Posted by grumpy View Post
    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?

    Quote Originally Posted by kmdv View Post
    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.

  10. #10
    Registered User
    Join Date
    Jun 2005
    Posts
    6,537
    Quote Originally Posted by jason_m View Post
    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.


    Quote Originally Posted by jason_m View Post
    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.

    Quote Originally Posted by jason_m View Post
    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.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy in reply to you, it is likely you deserve it. Suck it up, sunshine, and read this, this, and this before posting again.

  11. #11
    Registered User
    Join Date
    Aug 2010
    Location
    Poland
    Posts
    682
    Quote Originally Posted by grumpy View Post
    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.

    Quote Originally Posted by grumpy View Post
    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.
    Last edited by kmdv; 12-31-2012 at 02:47 AM.
    I never put signature, but I decided to make an exception.

  12. #12
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,308
    Quote Originally Posted by jason_m View Post
    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.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  13. #13
    Registered User
    Join Date
    Jun 2005
    Posts
    6,537
    Quote Originally Posted by kmdv View Post
    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++.

    Quote Originally Posted by km17dv View Post
    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.
    Quote Originally Posted by km17dv View Post
    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.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy in reply to you, it is likely you deserve it. Suck it up, sunshine, and read this, this, and this before posting again.

  14. #14
    Registered User
    Join Date
    Aug 2010
    Location
    Poland
    Posts
    682
    Quote Originally Posted by jason_m View Post
    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.

    Quote Originally Posted by jason_m View Post
    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.

    Quote Originally Posted by grumpy View Post
    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.

    Quote Originally Posted by grumpy View Post
    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.
    Last edited by kmdv; 12-31-2012 at 04:42 AM.
    I never put signature, but I decided to make an exception.

  15. #15
    Registered User
    Join Date
    May 2008
    Posts
    87
    Quote Originally Posted by jason_m View Post
    Sure enough, this resulted in a seg fault.

    ModelReader does not attempt to access any members of the Model passed in its constructor.
    Quote Originally Posted by iMalc View Post
    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.

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 2
    Last Post: 06-23-2012, 11:47 PM
  2. Replies: 4
    Last Post: 02-09-2012, 08:51 PM
  3. Replies: 4
    Last Post: 01-26-2012, 12:55 AM
  4. pointer conversion problems with a copy constructor
    By stanlvw in forum C++ Programming
    Replies: 8
    Last Post: 01-14-2008, 12:06 AM
  5. Copy constructor error, 'this' pointer conversion
    By bennyandthejets in forum C++ Programming
    Replies: 6
    Last Post: 01-08-2004, 05:18 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21