const correctness gone wrong I suspect...

Hi, I've been having a few headaches trying to get my code to compile. I feel like I've tried every way of changing "const"-ness, but still can't get this to compile with g++ in a cygwin environment.

Would you be so kind to tell me where on earth I am going wrong? Below is the code and the compiler error. The error talks about "qualifiers" which I assume mean there's a "const" missing somewhere, but for the life of me, I can't figure out where.

Code:

#include <iostream>

using namespace std;

class Test
{
   public:
     Test() : val(0) {}
     Test(int v) : val(v) {}

     int getVal() { return val; }

     bool operator<(const Test& rhs) {
       if (rhs.val < val) return false;
       else return true;
     }

   private:
     int val;
};


template <class T>
inline const T& mymax(const T& a, const T& b)
{
   if (a < b) return b;
   else return a;
}

int main(int argc, char* argv)
{
   int a1 = 10, a2 = 20;
   const Test t1(10); 
   const Test t2(20);

   int result = mymax(a1,a2);
   const Test result2 = mymax(t1,t2);

   cout << "Largest value of ints is: " << result << endl;
   cout << "Largest value of Test(ints) is: " << result2.getVal() << endl;
   
}

The compiler generates this error (g++):

Code:

maxTamplate.cpp: In function `const T& mymax(const T&, const T&) [with T = Test]`:
maxTemplate.cpp:37:    instantiated from here
maxTemplate.cpp:26: error: passing `const Test' as `this' argument of `bool Test::operator<(const Test&)' discards qualifiers

I've no idea what this message is trying to tell me

Code:

     int getVal() const { return val; }

     bool operator<(const Test& rhs) const {
       if (rhs.val < val) return false;
       else return true;
     }

The later will actually fix the problem. Remember that any getter--any function that does not change any class member variables--should be const.
The error is essentially trying to say that you are calling a non-const function (operator <) on a const object.

Many thanks Elysia. That's cleared up something I've read and vaguely know about, but have never implemented before. I appreciate
your help.

Incidentally, instead of writing:

Code:

if (rhs.val < val) return false;
else return true;

you can write:

Code:

return rhs.val >= val;

Originally Posted by laserlight

Incidentally, instead of writing:

Code:

if (rhs.val < val) return false;
else return true;

you can write:

Code:

return rhs.val >= val;

Which as it happens means that this classes less-than operator is incorrect, and thus cannot be used to store the item in a std::set for example. The problem is that for two equal items a and b, a<b and b<a would both return true. This is illogical and causes std::set to barf.
I suggest changing it to this:

Code:

       return val < rhs.val;

Originally Posted by matwsij

Code:

template <class T>
inline const T& mymax(const T& a, const T& b)
{
   if (a < b) return b;
   else return a;
}

a and/or b may be temporary objects and (an invalid) reference to an already destroyed object will be returned. Return by value.

I believe it is quite safe. A small experiment seems to confirm this:

Code:

#include <iostream>

class XTest
{
public:
	XTest(int n): m_n(n) { std::cout << "Constructing " << n << "...\n"; }
	~XTest() { std::cout << "Destructing " << m_n << "...\n"; m_n = 0xFFFFFFFF; }
	bool operator < (const XTest& rhs) const { return m_n < rhs.m_n; }
	int m_n;
};

template<typename T>
const T& max(const T& lhs, const T& rhs)
{
	if (lhs < rhs)
		return rhs;
	else
		return lhs;
}

int main()
{
	XTest t1(1), t2(2), t3(3);
	std::cout << "Max of t1 and t2 is: " << max(t1, t2).m_n << "\n";
	std::cout << "Max of t3 and t4 is: " << max(t3, XTest(4)).m_n << "\n";
}

(Yes, I know this is neither good practice nor portable; but then again, this is merely a quick and dirty test with the MSVC compiler; your mileage may vary.)
Output:

Constructing 1...
Constructing 2...
Constructing 3...
Max of t1 and t2 is: 2
Constructing 4...
Max of t3 and t4 is: 4
Destructing 4...
Destructing 3...
Destructing 2...
Destructing 1...

We clearly see that the lifetime of t4 is extended to after the statement.
I cannot be certain, though, as I am unsure exactly what the standard mandates about this.

The standard library uses the same signature for max as Elysia's code does and the only difference between Elysia's code and the code in question is use of inlining.

See section 12.2 point 5 of the C++0x standard final draft
In short, it is fine.

o_O

So it is correct, good to know.