Is this overly clever or unsafe?

Question

I was working on some code recently and decided to work on my operator overloading in c++, because I've never really implemented it before. So I overloaded the comparison operators for my matrix class using a compare function that returned 0 if LHS was less than RHS, 1 if LHS was greater than RHS and 2 if they were equal. Then I exploited the properties of logical not in c++ on integers, to get all of my compares in one line:

inline bool Matrix::operator<(Matrix &RHS){
  return ! (compare(*this,RHS));
}
inline bool Matrix::operator>(Matrix &RHS){
  return ! (compare((*this),RHS)-1);
}
inline bool Matrix::operator>=(Matrix &RHS){
  return compare((*this),RHS);
}
inline bool Matrix::operator<=(Matrix &RHS){
  return compare((*this),RHS)-1;
}
inline bool Matrix::operator!=(Matrix &RHS){
  return compare((*this),RHS)-2;
}
inline bool Matrix::operator==(Matrix &RHS){
  return !(compare((*this),RHS)-2);
}

Obviously I should be passing RHS as a const, I'm just probably not going to use this matrix class again and I didn't feel like writing another function that wasn't a reference to get the array index values solely for the comparator operation.

As per suggestion here is the code if Compare returns -1 for less, 0 for equal and 1 for positive.

inline bool Matrix::operator<(Matrix &RHS){
  return ! (compare(*this,RHS)+1);
}
inline bool Matrix::operator>(Matrix &RHS){
  return ! (compare((*this),RHS)-1);
}
inline bool Matrix::operator>=(Matrix &RHS){
  return compare((*this),RHS)+1;
}
inline bool Matrix::operator<=(Matrix &RHS){
  return compare((*this),RHS)-1;
}
inline bool Matrix::operator!=(Matrix &RHS){
  return compare((*this),RHS);
}
inline bool Matrix::operator==(Matrix &RHS){
  return !(compare((*this),RHS));
}

I don't know that this really increases the readability though.

Answer 1

Yes, it's overly clever - I read that code and have to think about why you're subtracting two from a function called compare. Don't make me think.

If you're being clever to make your code fit on one line then you've got muddled priorities. You should use as many lines as needed to make your code as clear as possible.

Programs must be written for people to read, and only incidentally for machines to execute. (Abelson & Sussman, Structure and Interpretation of Computer Programs)

Answer 2

As far as I can see it's safe, but it does take looking twice for everybody reading the code. Why would you want to do this?

Anyway, for comparison, all you ever need is < and either == or !=, the rest is canonical and I write it mostly by muscle memory. Also, binary operators treating their operands equally (they leave them alone) should IMO be implemented as non-members. Given this, plus using the sane comparison function (-1, 0, +1) and adding the necessary const, I come to this:

// doing real work
inline bool operator<(const Matrix& l, const Matrix &r)
{
  return -1 == compare(l,r);
}
inline bool operator==(const Matrix& l, const Matrix &r)
{
  return 0 == compare(l,r);
}
// canonical
inline bool operator> (const Matrix& l, const Matrix &r) {return r < l;}
inline bool operator>=(const Matrix& l, const Matrix &r) {return !(l < r);}
inline bool operator<=(const Matrix& l, const Matrix &r) {return !(r < l);}
inline bool operator!=(const Matrix& l, const Matrix &r) {return !(l == r);}

The comparisons might not be as clever as yours, but everyone who's ever seen strcmp() knows immediately what they do. Note that I even added 0 != compare(...), which is completely unnecessary - for the compiler. For humans IMO it makes it more clear what's going on than the implicit cast to bool. Plus it emphasizes the symmetry to operator<'s implementation.