Subtle error when using temporaries to get iterators to a STL container: how to avoid it?

Question

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 the getTheMap() 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.

If class X can't be changed, what do we have to work with? Can class X be hidden, forcing users to use something else, like a wrapper class? — Vaughn Cato, Mar 21 '12 at 14:02
@VaughnCato You are perfectly right :). I've added a note in the `EDIT` section at the end of the question. — Vincenzo Pii, Mar 21 '12 at 14:10
What is the reason for continuing to return by value? Returing a reference and reference to const will not break old code. — pmr, Mar 21 '12 at 14:15
You can't do anything, other than redesigning the class to not be crappy. If the user is not aware of this, they shouldn't write C++. Also, if you return the map by reference, you might as well make `_map` public. — Cat Plus Plus, Mar 21 '12 at 14:21
@pmr I have containers of `boost::shared_ptr`, I preferred returning it by value to avoid memory issues with multiple threads (http://stackoverflow.com/a/327581/528313) — Vincenzo Pii, Mar 21 '12 at 14:21
@CatPlusPlus yours is an acceptable answer, that's reasonable, exactly something I wanted to know. I wished there was some compiler flag I wasn't aware about, but, apparently, there isn't :). — Vincenzo Pii, Mar 21 '12 at 14:33

score 2 · Accepted Answer · answered Mar 21 '12 at 14:47

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.

Also known as returning a proxy object. – Mark Ransom Mar 21 '12 at 14:52 — Mark Ransom, Mar 21 '12 at 14:52

score 1 · Answer 2 · answered Mar 21 '12 at 14:57

1

Not in C++03. In C++11 the Standard library should already have such protection enabled.

answered Mar 21 '12 at 14:57

Puppy

144,682
38
256
465

At least this confirms my question has a sense :) – Vincenzo Pii Mar 21 '12 at 15:09

score 0 · Answer 3 · answered Mar 21 '12 at 15:19

0

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.

answered Mar 21 '12 at 15:19

gbjbaanb

51,617
12
104
148

Subtle error when using temporaries to get iterators to a STL container: how to avoid it?

3 Answers3