Memory leak or safe?

[beta3designs]

Hello, my learning of C++ has been constantly interrupted during the years and therefore sometimes I lack that better understanding of subjects I would like to have...

My question relates to two classes I have written for a project, pretty basic stuff as of now. I have a Node class and a Point class and the Node class has a private position_ member of type Point. Node has two methods one for setting the position and the other for getting it. (Point has the appropiate setters and getters for x and y).

My question is actually about my getPosition method implementation, I wanted to return a pointer to a Point object and so I wrote this piece of code:

Point* Node::getPosition(){
	Point* position = new Point();

	position->setX(position_.getX());
	position->setY(position_.getY());

	return position;
}

The question is... if I then use the Node class as follows:

float x;
Node node(0.0, 0.0, 0);
x = node.getPosition()->getX();

Could there be some memory leak or is this a safe way of working because I don't really know what happens with the pointer to a Point that node.getPosition() returns if I don't assign it, is it properly destroyed?

Thanks in advance!
P.S.: Tell me if you need more info about the classes and their implementations :)

[vlad from moscow]

It is a bad idea to allocate memory without any requirement. Of course your code contains a memory leak.

Either you should return a reference or a const reference to the member of type Point, or return a temporary object of type Point.
So it is much better to declare your method as

Point & Node::getPosition(){

	return position_;
}

const Point & Node::getPosition() const{

	return position_;
}

or as

Point Node::getPosition() const{
	
	return Point( position_.getX()), position_.getX()) );
}

provided that there is constructor Point( int, int )

[beta3designs]

Thank you! It seems logical... I actually thought that if I allocated memory that way then I should destroy it somewhere else and its usage wouldn't be that easy but I didn't really know how to solve this issue.

Now the problem is I want to keep access as simple as:

node.getPosition().getX()

But not letting the user do, for example:

node.getPosition().setX(2.0f)

Since there is already a setPosition() method and I want to keep things consistent. If I use the constant reference to Point I cannot access the x position as I mentioned but if I don't use the const version I can access the set method.

Can this be done simply or should I reconsider the usage?
Thanks in advance again :)

P.S.: There is, in fact, a Point(float, float) constructor.

[Catfish2]

@ vlad: you're returning a reference to a temporary in you second snippet.

Could there be some memory leak or is this a safe way of working because I don't really know what happens with the pointer to a Point that node.getPosition() returns if I don't assign it, is it properly destroyed?

Rule of thumb: if a new doesn't have a corresponding delete somewhere, then you have a memory leak.

The exception is when you use smart pointers: if your compiler is old, you may use std::auto_ptr otherwise std::unique_ptr.
http://cplusplus.com/reference/memory/auto_ptr/auto_ptr/
http://cplusplus.com/reference/memory/unique_ptr/unique_ptr/

[vlad from moscow]

@Catfish2
@ vlad: you're returning a reference to a temporary in you second snippet.

Thanks. It was a typo. I described what the function in the second variant shall return but used copy and past and forgot to remove the reference.

I have updated already the code.

[ne555]

If I use the constant reference to Point I cannot access the x position as I mentioned

that's a problem with Point::getX()
It should be const

[beta3designs]

Thanks ne555!
Yeah my function was const float Point::getX(); and not
const float Point::getX() const; hehehe
Now everything works as I wanted thank you very much! :)

Thanks to all of you!

[Catfish2]

That first const doesn't do anything in this case.