9 replies to this topic

[ptchaos]

Greetings,
been a long time in manage world and now I'm going back to C++.

Trying to do a small object management, but I'm having an error that I can't figure out.

Reduced the project to a smallest possible to show my issue:
(the identation got a bit messed up on copy/paste)

#include <iostream>
#include <vector>
using namespace std;
//
class cControl
{
public:
	virtual void Draw()
	{		
			cout << "Control" << endl;
	};
};
//
class cLabel: public cControl
{
	public:
		cLabel()
		{
		}
		//
		void Draw()
		{
			cout << "Label" << endl;
		}
};
//
class cControls
{
	public:
		vector<cControl*> listControls;
	  
		//
	cLabel CreateLabel()
	{
		cLabel label;
		// #1
		//this->AddControl(&label);
		return(label);
	}
		  
	//
	void AddControl(cControl *control)
	{
		this->listControls.push_back(control);
	}
	//
	void Draw()
	{
		for(cControl *ctrl: listControls)
			ctrl->Draw();
	}
};
//
int main()
{
	cControls controls;
  
	cLabel label1=controls.CreateLabel();
	// #2
	controls.AddControl(&label1);
	//
	controls.Draw();
	//
	cin.get();
	//
	return(EXIT_SUCCESS);
}

The point is to have a base control class that others will derive (label, textbox, line, panel, etc).
I've pointed the problem with #1 and #2.
If I add the control to the list in the main function (#2), it works fine and it calls Draw from label correctly.
But if I comment #2 and uncomment #1, so it adds automatically to list when creating label, it crashes when trying to call Draw from label.

From what I've checked, when the label is added to the vector inside the CreateLabel function, something changes when it gets out of that function, which makes unaccessible when calling the Draw.

Can anyone point me out to the correct path?

Thanks,
chaos

[SiCrane]

Your cLabel objects are created on the stack and you are storing pointers to them in the vectors. This is bad juju as they get destroyed when the function that contains them exits. Two possible fixes are either heap allocate them with new or store by value rather than by pointer.

[ptchaos]

Your cLabel objects are created on the stack and you are storing pointers to them in the vectors. This is bad juju as they get destroyed when the function that contains them exits. Two possible fixes are either heap allocate them with new or store by value rather than by pointer.

Awesome SiCrane, thank you so much.

I've changed to:

cLabel *CreateLabel()
{
  cLabel *pLabel=new cLabel();
  // #1
  this->AddControl(pLabel);
  return(pLabel);
}

Seems to be working fine now


Thanks,
chaos

[Trienco]

Of course now you have to ask yourself who is responsible for cleaning it up and when should that happen. Rule of thumb: for every "new" that isn't assigned to a smart pointer, there must be one (and only one) "delete".

[Servant of the Lord]

Seems to be working fine now

As Trienco points out, now you are creating memory that you are responsible to free, but you never free it.
In programming, something that "seems" to work, doesn't mean it doesn't have bad side effects.

So, either you need to A) manually micro-manage your memory, or B) Let C++ manage your memory for you using smart pointers.

If you are using C++11 (the newest C++ standard released last year), you have access to std::shared_ptr, std::weak_ptr, and std::unique_ptr. If you aren't using C++11, you probably still have access to them, but they'd be called std::tr1::shared_ptr, and etc...

You can find them in the #include <memory> header.

You can use them like this:

typedef std::shared_ptr<cControl> ControlPtr;
std::vector<ControlPtr> controls;

controls.push_back(std::make_shared<cLabel>("Label text"));

Why's every one of your classes start with the lowercase letter 'c', and what does it stand for? If 'c' stands for 'class', that's a little bit redundant don't you think?
(See: [CStupidClassName] Sometimes programmers see someone else do something, don't know why it's there, adopt the practice, and then spread that practice to other programmers)

[ptchaos]

Of course now you have to ask yourself who is responsible for cleaning it up and when should that happen. Rule of thumb: for every "new" that isn't assigned to a smart pointer, there must be one (and only one) "delete".

I'm deleting in the destructor of controls class. Don't know if its enough. Something like this:

~cControls()
   {
	for(cControl *ctrl : this->listControls)
	 delete ctrl;
   }

So, either you need to A) manually micro-manage your memory, or B) Let C++ manage your memory for you using smart pointers.

Yeah I know. I still have in my checkup list the use of smart pointers, since I've seen that's whats being used to ease memory management.

Why's every one of your classes start with the lowercase letter 'c', and what does it stand for? If 'c' stands for 'class', that's a little bit redundant don't you think?
(See: [CStupidClassName] Sometimes programmers see someone else do something, don't know why it's there, adopt the practice, and then spread that practice to other programmers)

LOL, I've been using that for a while, didn't knew it was a thing.
I usually use the c as a prefix for instanced classes and without the c for static (singleton).


Thanks for the comments guys,
chaos

[Servant of the Lord]

I'm deleting in the destructor of controls class. Don't know if its enough. Something like this:

~cControls()
   {
	for(cControl *ctrl : this->listControls)
	 delete ctrl;
   }

That's correct.

You might want to return cControl as a pointer from your 'Create' functions as well.

//Creates a label and returns a pointer to it. cControls retains ownership, and the label will be destroyed in cControls' destructor.
cLabel *cControls::CreateLabel()
{
	 cLabel *newLabel = new cLabel(); //Should be replaced with smart pointers in the future.

	 this->AddControl(newLabel);

	 return newLabel;
}

(Because code blocks are currently messing up, I posted the code here)

However, what if you want to pass parameters to the cLabel's constructor? Since you *are* using C++11 (your for-range loop), you can use C++11's perfect forwarding like this:

//Creates a label and returns a pointer to it. cControls retains ownership, and the label will be destroyed in cControls' destructor.
template<typename Arg>
cLabel *cControls::CreateLabel(Arg &&arg)
{
	 cLabel *newLabel = new cLabel(std::forward<Arg>(arg));
	 this->AddControl(newLabel);
	
	 return newLabel;
}

Code here

And you can then pass the cLabel constructor arguments (any number of them) directly to CreateLabel:

myControls.CreateLabel("My text", Color(255,0,0), Style::Bold);

That would internally call 'cLabel("My text", Color(255,0,0), Style::Bold)' without cLabel having to change anything about it.

But, you could even go a step further, and make your CreateLabel() work for any class that derives from cControl.

//Creates a cControl and returns a pointer to it. cControls retains ownership, and the label will be destroyed in cControls' destructor.
template<typename ControlType, typename Arg>
ControlType *cControls::Create(Arg &&arg)
{
	 ControlType *newControl = new ControlType(std::forward<Arg>(arg));
	 this->AddControl(newControl);

	 return newControl;
}

Code here

And you can use it like this:

myControls.Create<cLabel>("My text", Color(255,0,0), Style::Bold);
myControls.Create<cLineEdit>("Default text", Characters::Letters | Characters::Numbers | Characters::Symbols);
cMenu *menu = myControls.Create<cMenu>({"First item", "Second item", "Third item"});
menu->DoSomething();

Code here
...and not need to write a new CreateX() function for each type of control you make.

Those functions would've shared 90% of the same code anyway, and the 10% that is different can be handled by templates. Plus, if you ever wrap your code in a DLL, and later decide to create new cControl-derived classes specific to a certain project, you'd have to add new functions to cControls and recompile the DLL needlessly (with project-specific additions). But with this way, it's completely avoided.

Edited by Servant of the Lord, 05 November 2012 - 11:44 AM.

[rip-off]

I'm deleting in the destructor of controls class. Don't know if its enough.

It is a good start. Don't forget that if your class has a custom destructor, copy constructor or overloaded assignment operator it probably needs all three (this is referred to as the Rule of Three).


Unfortunately copying a class with polymorphic members is tricky. In addition sometimes it doesn't make sense to copy a class - doing so might be very expensive and probably indicate that you mean to pass by reference instead. To save you from the hassle of implementing this, you can also make your class "noncopyable". The best way to do this is to declare these functions as private, and do not implement them. Add a comment indicating that they are deliberately unimplemented.

Finally, if you offer clients the ability to remove elements from this list you will also have more work to do. In particular, correctly handling early returns and/or exceptions to avoid leaking memory.

[ptchaos]

/bow
Thanks for taking time to give suggestions, you guys are awesome.

That forward "thing" looks interesting. I need to invest more time in the new features.

~10 years in manage code and coming back feels a totally new language


Thanks again
chaos

[Matt-D]

Yeah I know. I still have in my checkup list the use of smart pointers, since I've seen that's whats being used to ease memory management.

If you're coming from a managed language (and/or aren't proficient/expert at manual memory management that really needs manual memory management), I'd recommend avoid using "new" (and, consequently, "delete") altogether. Make it your habit now, don't put it off (esp. since you need to break the habit of using "new" in your managed language -- note, that in addition to implying manual memory management, the heap allocation usually isn't the only (or even default) choice in C++, where stack-allocated values are common).

I suggest you read the following cover-to-cover, it's up-to-date modern C++11 (and feel free to ignore any materials on dynamic memory management in C++ which start by introducing new/delete before introducing smart pointers -- a pretty good way to tell they're already obsolete): http://www.informit.....aspx?p=1944072

After you read the above, the following is a good follow up:
http://herbsutter.com/gotw/_102/ // pay special attention to the "make_unique" part
http://herbsutter.com/gotw/_103/
http://herbsutter.com/gotw/_104/

In general, instead of using "new":
0. first consider if you shouldn't be using an automatic (stack-allocated) variable instead,

Note: thanks to std::reference_wrapper, the need for dynamic polymorphism is not a valid excuse to allocate on the heap:

// based on http://bryanstamour.com/2012/06/26/need-polymorphism/
Shape s1;
Shape s2;
Square sq1;
using shape_ref = std::reference_wrapper<Shape>;
std::vector<shape_ref> shapes = {
	std::ref<Shape>(s1),
	std::ref<Shape>(s2),
	std::ref<Shape>(sq1)
};
// Assume that we're calling the draw_shapes that
// takes in its argument by-reference.
std::for_each(
	begin(shapes),
	end(shapes),
	draw_shape
);

1. if not, and you don't need shared ownership (consider this as the default case), consider using make_unique,
// note: std::unique_ptr<T> has zero overhead over T* -- that's another reason for it to be your first choice for the heap-allocated variables; however, neither will be as fast as stack-allocated variables (provided you can fit them on the stack, of course -- the only limitation / possible excuse here)
2. if not, and you need shared ownership, use make_shared,
3. forget "delete" and "new" ever existed :-)

Edited by Matt-D, 05 November 2012 - 05:13 PM.