Crash, use it before construction

Question

I have a class, its object will be used by another class as following

class A
{
public:
    void F() {...}
};

class B
{
public:
    void G() { m_a->F(); ...} // use A's function

private:
    A* m_a;
};

Thus it is wrong to call B::G() before the assignment of pointer m_a. Is this acceptable design? Or there is a better way to do it.

The question comes from developing a GUI application. I have a window under the main window to show some basic information of current operation, such as what is happening, how long does it take, and so on. I put this window as a singleton for easy access by the operations which may scatter anywhere, and find the code crashes when closing the GUI application. The crash is due to the window is deleted after the deletion of GUI application (such as qApp in Qt). I then just put a reference pointer of the window in the singleton. And when the window is constructed, I set the reference pointer. The deletion of the window is controlled by the main window. Thus the above mentioned crash problem is solved. But if other developer uses the singleton before the window is constructed, the code will crash too. Any good way to solve it? Or we can just accept it because it is developer's mistake to use it before constructing? Thanks a lot!

How can it be an acceptable design? Invoking `G()` is undefined behavior. Maybe I'm not getting it — Andy Prowl, Jan 31 '13 at 15:48

Andy Prowl · Answer 1 · 2013-01-31T18:22:02.437

3

Invoking function G() may result in Undefined Behavior if m_a is not initialized, so you want to make sure that that's never the case. You have to change your code to make it look a bit like this:

class B
{

public:

    B() : m_a(nullptr) { } // m_a always initialized!

    bool G() 
    { 
        if (m_a == nullptr) return false; // No Undefined Behavior!
        m_a->F(); // This call is safe now
        ...
        return true;
    }

private:

    A* m_a;

};

And by the way, in general you should use smart pointers (choose the type which realizes the appropriate ownership semantics) rather than raw pointers, unless you have a good reason to do that. This would even save you from manual initialization (assuming shared ownership here):

#include <memory>

class B
{

public:

    bool G() 
    { 
        if (m_a == nullptr) return false; // No Undefined Behavior!
        m_a->F(); // This call is safe now
        ...
        return true;
    }

private:

    std::shared_ptr<A> m_a; // Automatically initalized to nullptr

};

edited Jan 31 '13 at 18:22

answered Jan 31 '13 at 15:52

Andy Prowl

124,023
23
387
451

2

There is not enough information in the problem statement to justify "you **should** use smart pointers...". Maybe "you **should consider** using smart pointers...". – Pete Becker Jan 31 '13 at 16:29
1

@PeteBecker: I know what you mean, but even the mere fact that smart pointers initialize themselves to `nullptr`, while raw pointers don't and require manual initialization, has a direct benefit in this example. Moreover, in Modern C++ one must have a good reason **not** to use smart pointers in order to prefer raw pointers to them. While it is true that these situations exist, I believe they do not represent the default case. And if the example does not mention the good reason for the choice of raw pointers, I feel entitled to mention that smart pointers should be used. Hence my wording. – Andy Prowl Jan 31 '13 at 17:17
Changing ownership semantics in order to not have to initialize something is inside-out design. – Pete Becker Jan 31 '13 at 17:59
@PeteBecker: my wording said "*smart* pointers", which includes `weak_ptr`, `unique_ptr`, and `shared_ptr`. And I stand by my statement: "you **should** use **smart** pointers". *Unless* there's a good reason for that. And of course one should choose the smart pointer which realizes the appropriate ownership semantics. My final example just picks one, I thought the message was clear enough without having to list all the possibilities. – Andy Prowl Jan 31 '13 at 18:10
Your example, because it uses `shared_ptr`, has different semantics from the original code. That's not a benign change. I would never recommend that beginners "use smart pointers" without analyzing what the effect of such a change is. – Pete Becker Jan 31 '13 at 18:19
@PeteBecker: I rephrased my wording, although I still believe the spirit of my previous words was quite clear. – Andy Prowl Jan 31 '13 at 18:23

score 2 · Accepted Answer · edited May 23 '17 at 12:04

2

You have to use the member initializer list to initialize m_a rather than assign it. It ensures you don't have to bother about getting m_a assigned before calling other functions.

edited May 23 '17 at 12:04

Community

1
1

answered Jan 31 '13 at 15:47

Alok Save

202,538
53
430
533

1

@OliCharlesworth: Yes, but if the OP is bothered about sequence of function calls by an actor, there are only two ways: 1. Either document it in interface document or 2. Initialize the pointer member during construction. – Alok Save Jan 31 '13 at 15:52

score 1 · Answer 3 · answered Jan 31 '13 at 15:53

Depending on your expected usage of the class, this can be an acceptable practice, but if you do that, you better change B::G() to check the pointer first:

void G() { if (m_a) m_a->F(); ...}

And make sure to initialize m_a in all constructors, at least to a null pointer if you don't currently have a real A to point at.

Crash, use it before construction

3 Answers3