Jump to content

  • Log In with Google      Sign In   
  • Create Account

We're offering banner ads on our site from just $5!

1. Details HERE. 2. GDNet+ Subscriptions HERE. 3. Ad upload HERE.


Error accessing vector member


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
9 replies to this topic

#1 ptchaos   Members   -  Reputation: 135

Like
0Likes
Like

Posted 04 November 2012 - 10:43 AM

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

Sponsor:

#2 SiCrane   Moderators   -  Reputation: 9662

Like
7Likes
Like

Posted 04 November 2012 - 10:51 AM

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.

#3 ptchaos   Members   -  Reputation: 135

Like
0Likes
Like

Posted 04 November 2012 - 12:30 PM

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

#4 Trienco   Crossbones+   -  Reputation: 2223

Like
4Likes
Like

Posted 04 November 2012 - 11:21 PM

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".
f@dzhttp://festini.device-zero.de

#5 Servant of the Lord   Crossbones+   -  Reputation: 20909

Like
4Likes
Like

Posted 04 November 2012 - 11:58 PM

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? Posted Image
(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)
It's perfectly fine to abbreviate my username to 'Servant' rather than copy+pasting it all the time.
All glory be to the Man at the right hand... On David's throne the King will reign, and the Government will rest upon His shoulders. All the earth will see the salvation of God.
Of Stranger Flames - [indie turn-based rpg set in a para-historical French colony] | Indie RPG development journal

[Fly with me on Twitter] [Google+] [My broken website]

[Need web hosting? I personally like A Small Orange]


#6 ptchaos   Members   -  Reputation: 135

Like
0Likes
Like

Posted 05 November 2012 - 02:34 AM

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? Posted Image
(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

#7 Servant of the Lord   Crossbones+   -  Reputation: 20909

Like
1Likes
Like

Posted 05 November 2012 - 11:28 AM

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. Posted Image

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. Posted Image

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

It's perfectly fine to abbreviate my username to 'Servant' rather than copy+pasting it all the time.
All glory be to the Man at the right hand... On David's throne the King will reign, and the Government will rest upon His shoulders. All the earth will see the salvation of God.
Of Stranger Flames - [indie turn-based rpg set in a para-historical French colony] | Indie RPG development journal

[Fly with me on Twitter] [Google+] [My broken website]

[Need web hosting? I personally like A Small Orange]


#8 rip-off   Moderators   -  Reputation: 8667

Like
4Likes
Like

Posted 05 November 2012 - 12:30 PM

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.

#9 ptchaos   Members   -  Reputation: 135

Like
0Likes
Like

Posted 05 November 2012 - 03:21 PM

/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 Posted Image


Thanks again Posted Image
chaos

#10 Matt-D   Crossbones+   -  Reputation: 1469

Like
3Likes
Like

Posted 05 November 2012 - 05:04 PM

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.





Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS