Subtle error when using temporaries to get iterators to a STL container: how to avoid it?
-
25-05-2021 - |
题
Let's consider this class:
class X {
std::map<uint32_t, uint32_t> _map;
public:
X() { /* Populate the map */ }
std::map<uint32_t, uint32_t> getTheMap() { return _map; }
};
and this buggy code:
X x;
// Statement 1
std::map<uint32_t, uint32_t>::const_iterator it = x.getTheMap().begin();
// Statement 2
std::map<uint32_t, uint32_t>::const_iterator et = x.getTheMap().end();
for (; it != et; it++) {
/* Access the map using the iterator it */
}
The wrong part is that, in Statement 1
and Statement 2
, I'm getting an iterator to a temporary object which is going to be destroyed at the end of each statement.
As a result the behavior inside the for()
loop is undefined.
A correct usage of the getTheMap()
method would have been this:
std::map<uint32_t, uint32_t> map = x.getTheMap();
std::map<uint32_t, uint32_t>::const_iterator it = map.begin();
std::map<uint32_t, uint32_t>::const_iterator et = map.end();
for (/* [...] */)
It must be noted that class X
has some serious design issues:
_map
should be better encapsulated inside the class (both for read and write access), so thegetTheMap()
method could be avoided- If the
getTheMap()
method is really needed, it could return a reference to_map
However, given class X "as is" (<-- see edit below), is there a way to prevent users from getting the iterator to the temporary?
EDIT: class X
can be changed, but getTheMap
method should exist and return by value. however I was also thinking about compiler warnings.
解决方案
One possibility would be to use a wrapper like this:
class X {
typedef std::map<uint32_t,uint32_t> Map;
Map _map;
struct MapWrap {
const Map &mapref;
MapWrap(const Map &mapref_arg)
: mapref(mapref_arg)
{
}
operator Map() const { return mapref; }
};
public:
MapWrap getTheMap()
{
return MapWrap(_map);
}
};
so that you get this:
X x;
std::map<uint32_t,uint32_t>::const_iterator iter = x.getTheMap().begin(); // error
std::map<uint32_t,uint32_t> m = x.getTheMap(); // no error
This prevents accidentally using the temporary like a map, but makes it where the user has to use a copy of the map.
其他提示
Not in C++03. In C++11 the Standard library should already have such protection enabled.
you could try forcing getTheMap() to return the original object by using std::move, but I'm not sure if that will work here.
If not, I guess returning a unique/shared_ptr of the member would be the best option.