Reputation: 4863
I have a problem with whether making class members constant or not. Let me give an example.
#include <iostream>
class ValueClass
{
int ival;
public:
void set(int i) {ival = i;}
int get() {return ival;}
};
class XXX;
class ABC
{
ValueClass vc;
public:
int getval() const {return vc.get() ;}
friend class XXX;
};
class XXX
{
std::vector<ABC> abclist;
void Invalidate()
{
// Iterates through abclist and modifies ValueClass members.
// e.g. abclist[i].vc.set(i);
}
};
class QWE
{
const ABC & abc;
public:
QWE(const ABC & abc_): abc(abc_) { }
const ABC & getABC() { return abc; }
};
int main()
{
ABC abc;
QWE qwe(abc);
std::cout << qwe.getABC().getval() << "\n"; // Compiler error
}
My ABC
class holds an instance of ValueClass which is responsible for having getter and setter for an int
value. Also QWE
class has an ABC
member and I need to obtain this member. I am told that returning the abc
as non-const is a very bad practice. But here comes the problem, I can not use non-const vc
in a const int getval()
function and I simply can not make it const
because in another thread XXX::Invalidate()
is called. This function changes the data in ValueClass with respect to some incoming data.
Obviously there is something wrong with my design, I can not blame the C++ language. How can I solve this problem?
Upvotes: 1
Views: 180
Reputation: 354969
As others have noted, the accessors should be const-qualified. As a general rule, if something can be const-qualified, it should be const-qualified (there are a few exceptions, of course, e.g. the return type of a function that returns by value should not be const-qualified, though it usually doesn't matter there).
However, there is another issue with the code as written: it is very easy to use incorrectly and accidentally lose track of your object lifetimes. Consider if you had instead declared qwe
in main()
as:
QWE qwe(ABC());
The ABC()
constructs a temporary ABC
object and passes it to the QWE
constrcutor. Since the constructor parameter abc_
is a const ABC&
, it will bind to that temporary object. You then bind the member reference abc
to the temporary object via the initializer abc(abc_)
. The constructor then returns.
After the constructor returns, the temporary ABC
object is destroyed and qwe.abc
is a dangling reference: it no longer refers to an object.
If a class is going to hang onto a reference that you pass to it, you should prefer to use a pointer for the parameter type: this way, it is clearer that you have to concern yourself with potential lifetime issues. In addition, reference-type class members are usually more mess than they are worth, since they make the class non-assignable (since references themselves are not assignable).
Upvotes: 1
Reputation: 91260
class ValueClass
{
...
int get() const {return ival;}
};
class QWE
{
...
const ABC & getABC() const { return abc; }
};
Your code must be const-correct throughout or you'll run into this problem all the time.
Upvotes: 0
Reputation: 89142
Make get() const in ValueClass
class ValueClass
{
int ival;
public:
void set(int i) {ival = i;}
int get() const {return ival;} // make get const
};
Probably should do the same for getABC
const ABC & getABC() const { return abc; }
This means that this method does not change the object it is called on.
Upvotes: 3