Overloading + operator

Question

I'm trying to familiarize myself with operators in C++. I figured I would do so with a simple case of vector addition. Unfortunately, I seem to have run into some issues. My class definition is as below:

#ifndef _MVEC_H_ #define _MVEC_H_ #include "Error.h" //I define things like throw(message) here, it works and is not the issue class MVec { private: double vec[3]; public: MVec(); MVec(double &); MVec(double *); MVec(MVec &); MVec & operator=(MVec &); inline double & operator[](const int i); inline const double & operator[](const int i) const; MVec operator+(const MVec &) const; ~MVec(); }; MVec::MVec() {} MVec::MVec(double &a) { for(int i = 0; i < 3; i++) vec[i] = a; } MVec::MVec(double *a) { for(int i = 0; i < 3; i++) vec[i] = *a++; } MVec::MVec(MVec &rhs) { for(int i = 0; i < 3; i++) vec[i] = rhs[i]; } MVec & MVec::operator=(MVec &rhs) { if(this != &rhs) for(int i = 0; i < 3; i++) vec[i] = rhs[i]; return *this; } inline double & MVec::operator[](const int i) { #ifdef _CHECKBOUNDS_ if(i < 0 || i >= 3) throw("Subscript out of bounds"); #endif return vec[i]; } inline const double & MVec::operator[](const int i) const { #ifdef _CHECKBOUNDS_ if(i < 0 || i >= 3) throw("Subscript out of bounds"); #endif return vec[i]; } MVec MVec::operator+(const MVec &vec1) const { MVec ans; for(int i = 0; i < 3; i++) ans[i] = vec[i] + vec1[i]; return ans; } MVec::~MVec() { delete[] vec; } #endif

The [] operator appears to work as intended. Unfortunately, the vector addition operator does not. Specifically, when I run the code:

#include "Error.h" #include "MVec.h" #include <cstdlib> #include <iostream> int main(int argc, char *argv[]) { MVec a, b, c; a[0] = 1; a[1] = 2; a[2] = 3; b[0] = 5.9906; b[1] = 72.1139; b[2] = 83.1324; //c = a + b; std::cout << (a + b)[0] << std::endl; std::cout << (a + b)[1] << std::endl; std::cout << (a + b)[2] << std::endl; exit(0); }

When I uncomment the line c = a + b; I get a compiler error:

no match for 'operator=' in 'c = MVec::operator+(const MVec&) const(((const MVec&)((const MVec*)(& b))))'

When I comment it out, I get a glibc detected error after the first std::cout. Presumably, I am doing something wrong with the temporary variable I am creating in the operator+ function. Unfortunately, I am not (quite) smart enough to figure out what. Any and all insight into this would be very helpful.

I think that your assignment operator needs to take a reference to a const instance: MVec & operator=(MVec const &);, since temporaries (such as those returned by the a + b expression) are implicitly const. That should solve your compile-time error. — cdhowie
– cdhowie, Commented Nov 19, 2012 at 19:53
It looks like you have some weird characters in those compiler errors, try recopying. — user1599559
– user1599559, Commented Nov 19, 2012 at 19:53
@Kyryx: Not weird. Just copy/paste artifacts from GCC. They're quotations. — Lightness Races in Orbit
– Lightness Races in Orbit, Commented Nov 19, 2012 at 19:57

juanchopanza · Accepted Answer · 2012-11-19 20:01:46Z

You need to take a const reference to MVec in your copy constructor in order to be able to use it with temporaries:

MVec(const MVec &);

The same applies to the assignment operator, and the constructor taking a double:

MVec(const double &); // or no reference, MVec(double); MVec& operator=(const MVec& rhs);

You should also remove the delete [] vec from the destructor, because vec is not dynamically allocated. This is the likely cause of the glibc error.

Now, in order for expressions such as

SomeVec = 1.0 + SomeOtherVec;

you need to declare the operator+ as a non-member fumction:

MVec operator+(const MVec& lhs, const MVec& lhs);

This will allow for implicit conversions both on the LHS and on the RHS. In general it is a good idea to have these types of operators as non-member functions, to guarantee symmetry between LHS and RHS operands.

On the other hand, it probably makes more sense not to allow the implicit conversions from double at all. You can achieve this by making the relevant constructor explicit:

explicit MVec(double);

I would suggest altering the double & constructor in a different way -- drop the reference entirely. You don't really get any benefit by using references for such small types; they only serve to add an unnecessary level of indirection (when inlining is not possible, that is).
@cdhowie I added a comment. I don't see it as an important issue (although I would have not used a reference).
@juanchopanza Nope, it's not an important issue at all, just thought I would point it out.

Dietmar Kühl · Accepted Answer · 2012-11-19 20:00:02Z

The compile time error is easily explained: your assignment operator expects a non-const reference as an argument but you cannot bind a non-const reference to a temporary as it is returned from your operator+(). The kind of naive fix is to make the operator take a const& instead. However, you actually don't need to define a copy assignment operator at all! The compiler generated copy construct, copy assignment, and destructor are all just fine. You can just remove them and you are better off.

Removing, in particular, the destructor would also fix your other problem: you are delete[]ing memory in your destructor you haven't allocated! You should never do this. That is, if you fixed your destructor it would be empty, i.e., you can just remove it as well.

evanmcdonnal · Accepted Answer · 2012-11-19 20:01:53Z

this

MVec MVec::operator+(const MVec &vec1) const { MVec ans; for(int i = 0; i < 3; i++) ans[i] = vec[i] + vec1[i]; return ans; }

should be defined as a non-member function with the this MVec operator+(const MVec &vec1, const MVec &vec2) const definition. then you need to modify to add vec1[i] + vec2[i], assuming your goal is to add every value in the two vectors.

Also, you should add bounds checking. If either vector's length is less than three you're going to crash. You should either add up to the length of the shorter vector or not add them at all of the lengths aren't the same. example

 int loopVar = 0; if (vec1.length() > vec2.length()) loopVar = vec2.length(); else loopVar = vec1.length(); for (int i = 0; i < loopVar; i++) ans[i] = vec1[i] + vec2[i];

No, this is an implicit argument. The OP's operator+ overload should indeed work. (Or, anyway, the argument list is not the problem.)
Even more, member operator+, that takes 2 arguments is invalid
@cdhowie sorry that has been edited. Sloppy post. However, the second part is important.
In general, I agree, I should check vector lengths to ensure that they are the same, but since I've explicitly set the length of all vectors in this class to be 3, it shouldn't matter, right?
@wolfPack88 That is true. And also, if you're absolutely certain that will be the length you'll have better performance by ignoring those general cases.

Collectives™ on Stack Overflow

Overloading + operator

3 Answers 3

5 Comments

Comments

5 Comments

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

5 Comments

Comments

5 Comments

Related