Can assigning a member variable (CBrush *) to memory dynamically allocated in a class constructor with new cause problems?

Question

Here is a stripped down version of my class setup:

class CMyClass : public CDialog
{
    CMyClass(CWnd *pParent = NULL); // constructor
    ~CMyClass();
    _ CBrush *_pRadBkgColor; // background color of a radio button
}

CMyClass::CMyClass(CWnd *pParent /*=NULL*/)
{
    // assign CBrush pointer to a new brush
    _pRadBkgColor = new CBrush(RGB(0xFF, 0xFF, 0xFF));
}

CMyClass::~CMyClass()
{
    if( _pRadBkgColor != NULL )
    {
        delete _pRadBkgColor
    }
    _pRadBkgColor = NULL;
}

Now, when I run a tool that parses code for subtle errors, I get this:

new in constructor for class 'Name' which has no assignment operator -- Within a constructor for the cited class, there appeared a new. However, no assignment operator was declared for this class. Presumably some class member (or members) points to dynamically allocated memory. Such memory is not treated properly by the default assignment operator. Normally a custom assignment operator would be needed. Thus, if x and y are both of type Symbol x = y; will result in pointer duplication. A later delete would create chaos.

I believe it's telling me that if I have two member variables that are CBrush pointers, lets call them a and b, and I initialize a with new in my constructor, then later I say b = a (or assign a to any other address really... I guess it'd be best to make that constant), and then I delete a or b, there will be chaos.

If I do no such assignment, is this safe?

Thanks Steven

You question is unclear because you show code and then talk about general stuff that is not necessarily in your code. For CMyClass you need to also declare and define an assignment operator and a copy constructor due to the pointer. Or you can declare them private and don't implement them. — Anon Mail, May 03 '12 at 22:09
Also, get rid of the NULL check and setting your pointer to NULL after the delete. Just do a delete. Also, it's good practice to use the initialization list when possible instead of the constructor body. In this case it doesn't matter performance-wise but it's good practice. — Anon Mail, May 03 '12 at 22:11
Thanks, I will get used to using the initialization list. I've only ever used it when initializing members to passed or literal numbers. I did not realize I could throw a function call (including constructor with `new`) and still get the results I wanted. Though I didn't bother to look much either. The general stuff was output from a code parser and pertains to the sample code I provided. hmjd got it. Thank you for your assistance :) — ZayJay, May 04 '12 at 07:17

score 1 · Answer 1 · edited May 23 '17 at 11:49

It is warning that if a copy of CMyClass is made then two instances of CMyClass both point to the same CBrush. If one of the CMyClass instances is destructed then the other instance is left with a dangling pointer to a CBrush as the destructed CMyClass deleted it.

If you have a dynamically allocated member then you need to implement a copy constructor and assignment operator to correctly copy the dynamically allocated member or make the class non-copyable by declaring the copy constructor and assignment operator private:

class CMyClass : public CDialog
{
public:
    CMyClass(CWnd *pParent = NULL);
    ~CMyClass();
private:
    CBrush *_pRadBkgColor;
    CMyClass(const CMyClass&);             // Copy constructor.
    CMyClass& operator=(const CMyClass&);  // Assignment operator.
};

See What is The Rule of Three?.

This. Thank you very much. I came across the Rule of Three, but did not put two and two together apparently. Thank you for your answer. — ZayJay, May 04 '12 at 07:26

score 0 · Answer 2 · answered May 03 '12 at 22:13

0

The check _pRadBkgColor != NULL in the destructor is unnecessary. delete is callable with a NULL pointer. The following will suffice:

CMyClass::~CMyClass() 
{
    delete _pRadBkgColor 
    _pRadBkgColor = NULL; 
}

answered May 03 '12 at 22:13

damienh

164
2
8

Yeah, I was just told it was good practice by a higher-up. And actually, at a job interview recently, on a code snippet in a quiz they did a pointer check before deleting it. `...if(ptr){ delete ptr; }...`I agree with you, too though. – ZayJay May 04 '12 at 07:23
There is absolutely no need to assign NULL to your pointer after you do a delete. You're in the destructor so once it ends the class and it's members cease to exist. – Anon Mail May 04 '12 at 12:30

CuriousGeorge · Answer 3 · 2012-05-04T10:58:28.040

0

There is nothing wrong with your code. If an object is allocated with new, then it MUST be explicitly freed by calling delete. Your code does exactly that.

Also, there is an idiom called RAII which further describes why what you are doing is correct.

However, stack-allocated variables are often faster, and you can sidestep the entire issue with the following code:

class CMyClass : public CDialog
{ 
  CMyClass(CWnd *pParent = NULL);
  ~CMyClass();
 _ CBrush _pRadBkgColor;
}

CMyClass::CMyClass(CWnd *pParent /*=NULL*/)
      : _pRadBkgColor(RGB(0xFF, 0xFF, 0xFF))
{
  HBRUSH hBr = _pRadBkgColor; // no problem, conversion operator.
}

CMyClass::~CMyClass()
{

}

edited May 04 '12 at 10:58

answered May 03 '12 at 22:43

CuriousGeorge

7,120
6
42
74

Thanks @Nick. I appreciate the sidestep, but the code that I _didn't_ show involves a casting `_pRadBkgColor` to an HBRUSH to conform to an MFC function prototype. (What's the word for that again? If this was jeopardy, I'd say, "The return type, parameter list, and name of the function make up this fundamental computer science idiom.") – ZayJay May 04 '12 at 07:25
CBrush has an HBRUSH conversion operator: http://msdn.microsoft.com/en-US/library/ye45d4c7(v=vs.80).aspx =) – CuriousGeorge May 04 '12 at 10:54
Ah, I did not catch that before. Interesting note from the link: In the example, it calls `FillRect(hDC, &rc, (HBRUSH)(COLOR_BTNFACE+1));`, and `FillRect` is set up like so: `int FillRect(HDC hDC, CONST RECT* lprc, HBRUSH hbr);` So am I right in assuming that in the example, a `COLORREF` is being passed as the third parameter, and that `COLORREF` is being used as input to a `CBRUSH` constructor, which is in turn being cast to an `HBRUSH` for valid `FillRect()` input? Is all of that happening behind the scenes? – ZayJay May 04 '12 at 16:32
No :) HBRUSH is defined as "DECLARE_HANDLE(HBRUSH);" which expands to "struct HBRUSH__{int unused;}; typedef struct HBRUSH__ *HBRUSH". So you can see that HBRUSH is a pointer to a struct, containing an integer. That integer is likely an index into GDI's internal storage of active brushes, but the GDI could also be casting another pointer as an int and storing it there as well. For FillRect: even though it says it takes an HBRUSH, GDI can easily figure out it's a system color index instead by checking it's range. See GetSysColor(). – CuriousGeorge May 05 '12 at 19:45

Can assigning a member variable (CBrush *) to memory dynamically allocated in a class constructor with new cause problems?

3 Answers3