0

can you explain me why this code doesn't work. operator+ override:

Fraction& Fraction::operator+(const Fraction& f) {
    Fraction temp;
    if (this->denominator == f.denominator){
        temp.numerator = this->numerator + f.numerator;
        temp.numerator = this->numerator;
        temp.simplifier();
    }
    else {
        temp.numerator = this->numerator * f.denominator + f.numerator * this->denominator;
        temp.denominator = this->denominator * f.denominator;
        temp.simplifier();
    }
    return temp;
}

operator= override:

void Fraction::operator=(const Fraction& f) {
    this->numerator = f.numerator;
    this->denominator = f.denominator;
}

after code

Fraction res;
res = f + g;

fields of res stay uninitialised. But, for example, code 

Fraction res = g;

is working properly. So operator= doesn't understand (f + g) as one object? Thanks.

Roman Korol
  • 21
  • 5
  • 1
    Compiler should warn about this; but with ```Fraction::operator+``` you're returning a reference to a local variable. – Jorn Vernee Mar 30 '16 at 08:55
  • 1
    Your `operator+` returns a reference to a local variable, which is undefined behavior. `Fraction res = g;` is an initialization, it won't result in a call to `operator=`. – user657267 Mar 30 '16 at 08:55
  • Fyi one of the best questions and answer-stacks on this site, [**Operator Overloading**](https://stackoverflow.com/questions/4421706/operator-overloading). It's worth the read. – WhozCraig Mar 30 '16 at 08:57

4 Answers4

4

The problem is that your overload is returning a reference to an object, temp, which is destroyed when the function returns.

Accessing that object after the function returns is undefined.

Return by value instead:

Fraction Fraction::operator+(const Fraction& f)

And

Fraction res = g;

isn't an assignment but an initialisation and will not use your assignment operator.

molbdnilo
  • 64,751
  • 3
  • 43
  • 82
  • 1
    Specifically, `Fraction res = g;` will use the copy construction if you have defined one, or a compiler supplied one if you haven't. (I suspect the compiler supplied one will be fine.) – Martin Bonner supports Monica Mar 30 '16 at 09:02
0

You're returning a reference to a temporary that was created on the stack. "temp", in operator+, has a lifetime that ends when operator+ returns. Its contents will likely be altered in chaotic ways, the next time a method is called.

Sniggerfardimungus
  • 11,583
  • 10
  • 52
  • 97
0

In addition to return a reference to a temporary, the other problem you have is:

    if (this->denominator == f.denominator){
        temp.numerator = this->numerator + f.numerator;
        temp.numerator = this->numerator;  // *HERE*
        temp.simplifier();
    }

I am pretty sure the line labelled *HERE* should be:

        temp.denominator = this->denominator;

Otherwise you will call simplifier with an uninitialized denominator and bad things will happen.

I would also recommend making operator + a free standing binary operator which takes two Fraction arguments - and make it use operator +=. Note: lhs is passed as a copy, not as a const reference.

Fraction operator +(Fraction lhs, const Fraction &rhs)
{
    lhs += rhs;
    return lhs;
}
Martin Bonner supports Monica
  • 28,528
  • 3
  • 51
  • 88
0

If you want to return "temp" as reference then you have to declare "temp" as:

static Fraction temp;

And now everything will work. Why because appending static keyword before declaration made "temp" available until program termination..

Shiv
  • 122
  • 2
  • 16