overloaded operators returning wrong values

[andrew browning]

i have overlaoded all of my arithmetic operators but all are
functioning as multiplication. below is a sample of the addition
operator:

Rational operator + (const Rational& r1, const Rational& r2){
//Postcondition: sum of r1 and r2 are returned

int numerator;
int denominator;

denominator = (r1.getDen() * r2.getDen());
numerator = (r1.getNum() * r2.getDen() + r2.getNum() *
r1.getDen());
Rational sum (numerator, denominator);

return sum;
}

[Kai-Uwe Bux]

Axter wrote:

[snip]

I've reread you're previous post on this subject, but I don't see where
you're giving a valid reason why Herb Sutters recommend method should
not be recommended as a general rule.
Right. I just gave examples where it might be better/easier to go the other
way. Given the closing remark of your current posting, I see now that I was
reading too much into the recommendation. Sorry.

However, in the course of this conversation, I have been thinking about this
issue more carefully; and now I feel prepared to actually provide a reason
why the general recommendation should be to define operator@= in terms of
operator@ and operator=. I think it is less error prone than the
Sutter/Alexandrescu way.

The example of the OP can be used to illustrate this. Let us have a look at
a rational number class:

class rational {

long n; // numerator
long d; // denominator

public:

// ... stuff

};

Ignoring reduction to lowest possible terms, the Sutter/Alexandrescu way to
go about addition is:

rational & operator+= ( rational & lhs, rational const & rhs ) {
lhs.n = lhs.n * rhs.d + lhs.d * rhs.n;
lhs.d *= rhs.d;
return ( lhs );
}

rational operator+ ( rational const & lhs, rational const & rhs ) {
rational result ( lhs );
result += rhs;
return ( result );
}
This is easy to get wrong:

rational & operator+= ( rational & lhs, rational const & rhs ) {
lhs.d *= rhs.d;
lhs.n = lhs.n * rhs.d + lhs.d * rhs.n; // wrong: uses new lhs.d !
}

The archives show posts dealing with re-implementations of std::complex that
suffer from this kind of bug. And I myself found myself recently falling
for this while working on a matrix class (maybe that is why I am so
sensitive to this issue right now): I was using entries that I had just
overwritten.

On the other hand,

rational operator+ ( rational const & lhs, rational const & rhs ) {
rational result ( lhs.n * rhs.d + lhs.d * rhs.n, lhs.d * rhs.d );
return result;
}

rational & operator+= ( rational & lhs, rational const & rhs ) {
lhs = lhs + rhs;
return ( lhs );
}

is much less prone to this kind of bug. (Come to think of it, this might be
the reason that Sutter and Alexandrescu mention the complex number class as
a case where they would recommend the second approach.)

Your followup post suggested the opposite approach using swap method.
Actually, I wrote that in my first post on this topic. In my follow up, I
elaborated on expression templates.
But not all classes have swap method, and infact, most don't. Those
that do, don't always have an efficient swap method.
True, in that case, you would use assignment. I suggested the swap method
because of the examples that I mentioned: matrix multiplication and
arbitrary precission operations. In both cases, a swap method *should* be
there and more efficient than assignment.

I'm sure there are times when using the opposite approach would be more
efficient, but in general, I would recommend Herb Sutters method.
As far as efficiency is concerned, you are right. However, I would remark
that this optimizes operator@= only: operator@ will not get any faster than
a direct implementation. Also, I would contend that expression templates
are yet more efficient to eliminate unwanted temporaries (altough the added
complexity to the code is considerable and for this reason I would not
recommend this as a general approach). Anyway, as a measure to optimize
performance, I would agree with you.

However, as a general guideline I would rather recommend the following
procedure:

1) First, write operator@= in terms of operator@ and operator= because
that is the most easy version to get right.
2) If profiling shows a need for optimization, refactor operator@=. Use
your operator@ code for inspiration.
3) Run tests to check whether the semantics agree. This should catch
bugs like the one above.
4) Finally rewrite operator@ following the Sutter/Alexandrescu approach.
This eliminates code dubblication.

The Sutter/Alexandrescu recommendation, to me, looks a little bit like
premature optimization.

Remember, that a general rule is for general purposes. It doesn't mean
that the rule is required to be followed for every requirement.

Thanks for reminding me. I tend to forget about caveats that always apply.
Best regards

Kai-Uwe Bux