Archived

This topic is now archived and is closed to further replies.

c_wraith

[C++] operator overloading and memory management.

Recommended Posts

c_wraith    122
Consider a class with overloaded arithmetic operators, following the immutable pattern. I seem to have embedded my question in the code:
  
class Vector2d
{
private:
    double m_x, m_y;
 
public:
    Vector2d(double x, double y) { m_x = x; m_y = y; }
    virtual ~Vector2d() {}
 
    double getX() { return m_x; }
    double getY() { return m_y; }
 
    const Vector2d& operator+ (const Vector2d& other)
    {
        // How do I do this without a memory leak?

        //

        // I get compiler warnings on this:

        
        return Vector2d(m_x + other.m_x, m_y + other.m_y);
        
        // The compiler warnings are obviously because I''m

        // returning a reference to a stack allocated

        // structure, which will get blown away on the next

        // function call.

        //

        // The only alternative I can think of is this:

        
        return *new Vector2d(m_x + other.m_x, m_y + other.m_y);
        
        // While the above compiles without warnings, it seems

        // like a memory leak.  Does the compiler handle this

        // somehow, or is this a genuine memory leak?

        //

        // If it is a memory leak, what''s the correct solution

        // to this problem?

    }

    // other operators

}  

Share this post


Link to post
Share on other sites
Guest Anonymous Poster   
Guest Anonymous Poster
The answer is simple, don''t return a reference, returning a
reference when creating a new object never works, instead use
this form:

class Vector2D
{
... etc ...

Vector2D operator+ (const Vector2D &v) {
return Vector2D(m_x + v.m_x, m_y + v.m_y);
}
};

Share this post


Link to post
Share on other sites
Mark_C    122
I believe the correct way looks like this.

const Vector2d& operator+ (const Vector2d& other)
{
static Vector2d retv(0,0);
retv = Vector2d(m_x + other.m_x, m_y + other.m_y);
return retv;
}

Share this post


Link to post
Share on other sites
Fruny    1658
quote:
Original post by Mark_C
I believe the correct way looks like this.

const Vector2d& operator+ (const Vector2d& other)
{
static Vector2d retv(0,0);
retv = Vector2d(m_x + other.m_x, m_y + other.m_y);
return retv;
}


If you do that, you have this problem :

Vector2d a,b,c,d;
...
(a+b) == (c+d) will always be true, since both (a+b) and (c+d) 'refer' to the same static variable 'retv'. So, say, (a+b) first sets the value of retv, and returns a reference to retv, then (c+d) changes the value of retv, and returns a reference to retv... obviously retv == retv.




Edited by - Fruny on December 13, 2001 2:18:33 PM

Share this post


Link to post
Share on other sites
null_pointer    289
quote:
Original post by

While the above compiles without warnings, it seems like a memory leak. Does the compiler handle this somehow, or is this a genuine memory leak?



Technically, no. If you call delete on the address of the const reference to Vector2d, then everything will be fine. However, it does not make very much sense, and writing an arithmetic operator that creates objects on the free store is asking for trouble.

Now I have a couple of points to make regarding the efficiency of your class.

First, if you are not deriving classes from Vector2d that require destructors called by classes using them as a base class for other classes which themselves require destructors, and calling delete on base class pointers within the hierarchy, then the virtual destructor is unnecessary.

Why mention this? Because if it is a vector class, then chances are that you will be creating a lot of them. Classes that have virtual member functions have an added per-instance cost, which is usually the size of a single pointer. If the virtual destructor is the only virtual member function and is unnecessary, then removing it will give you a smaller vector class. (FYI, the size of a pointer is four bytes on Win32.)

(And if you do not need a virtual destructor, then defining an empty destructor is equally unnecessary and may prevent the compiler from performing other optimizations, especially given the return-by-value nature of your arithmetic operators, which implies destructor calls.)

Second, when you assign values to member variables within a constructor, the preferred syntax is:

Vector2d(double x, double y) : m_x(x), m_y(y) {}


Although your method will work as well. The exception to this rule is to not directly allocate objects on the free store until you get inside the blocks, otherwise you can cause memory leaks if exceptions occur.

Third, if you do not need the constructor call then I would eliminate that in favor of speed. If you allocate an array of one thousand vectors than that will be one thousand constructor calls, followed by the initialization that you will need to do anyway. (I do not know of anyone who needs an array of one thousand vectors with the same values: 0,0.). You can eliminate the requirement of calling a constructor by adding an empty default constructor:

Vector2d() {}


- null_pointer

Share this post


Link to post
Share on other sites
Stoffel    250
This is why, by the way, vectors and matrices are a bad subject for classes--the typical first try yields huge amounts of temporaries and copies. To do this "the right way", you should have vector''s operators return proxies of their operations, and the assignment operator evaluate the proxies. This gets extremely complicated.

Share this post


Link to post
Share on other sites
c_wraith    122
Hmm... Too many replies that are FAR off topic. I was just using this code as an example. The question was "How do I manage that memory correctly?"

Are you telling me its simply impossible to use the immutable pattern correctly with overloaded arithmetic operators in C++?

Share this post


Link to post
Share on other sites
Stoffel    250
No, we''re saying return by value, not by reference.

Read "Effective C++" by Meyers. He goes through how to write a class (a string class for his example) with correct operator overloads so that it works just like a built-in type.

Share this post


Link to post
Share on other sites
null_pointer    289
I thought that the const reference scoping rules only applied to function parameters, not return values, and only so that you could pass temporaries to functions expecting const references:

#include 

using std::string;

void dosomething(const string& s)
{
// ...
}

int main()
{
dosomething(string("hello"));
return 0;
}

Share this post


Link to post
Share on other sites
Stoffel    250
quote:
Original post by null_pointer
I thought that the const reference scoping rules only applied to function parameters, not return values, and only so that you could pass temporaries to functions expecting const references:


Not sure what you mean by "scoping rules", but I have member functions return objects by const reference all the time.
  
class ConstSingleton
{
ConstSingleton (); // private property, keep out

public:
const ConstSingleton &getInstance () const
{
static ConstSingleton theObj;
return theObj;
}
};

Doesn''t have to be singleton of course, but almost anywhere I return an object that''s guaranteed to exist, I return it by const reference. (If it might not exist, I return a pointer and return NULL when it doesn''t exist).

Share this post


Link to post
Share on other sites