Don’t trust the temporaries (C++)!

Class X is defined as follows (C++):


class X {
    std::map<uint32_t, uint32_t> _map;
public:
    X() {
        for (uint32_t i = 0; i < 20; i++) {
            _map[i] = i + 10;
        }
    }

    std::map<uint32_t, uint32_t> getTheMap() const {
        return _map;
    }
};

The getTheMap method returns by value.

What is the output of this code?

X x;
std::map<uint32_t, uint32_t>::const_iterator it = x.getTheMap().begin();
std::map<uint32_t, uint32_t>::const_iterator et = x.getTheMap().end();

for (; it != et; it++) {
    std::cout << "[" << it->first << ";" << it->second << "]" << std::endl;
}

It’s undefined!

At lines 2 and 3, the map iterators are obtained on a temporary object which is destroyed just at the end of the statement. That’s because getTheMap() returns by value. Consequently, on the for loop at line 5, we are iterating over unallocated memory.

My main concern about class X code was to find a way to stop users using it the wrong way (such as in the above code sample). I was told on stackoverflow that in C++11 the STL itself should have a protection to make that code illegal.

Another interesting and very elegant solution is the use of a wrapper:

class X {
    std::map<uint32_t, uint32_t> _map;

    class InternalWrapper {
        const std::map<uint32_t, uint32_t>& _internalMap;

    public:
        InternalWrapper(const std::map<uint32_t, uint32_t>& map) :
            _internalMap(map)
        {}

        operator std::map<uint32_t, uint32_t>() const {
            return _internalMap;
        }

    };

public:
    X() {
        for (uint32_t i = 0; i < 20; i++) {
            _map[i] = i + 10;
        }
    }

    InternalWrapper getTheMap() const {
        return InternalWrapper(_map);
    }
};

The magic of this code is done by the automatic type conversion operator at line 12.

Now, the getTheMap() method returns an object of type InternalWrapper by value. This object can be implicitly converted to std::map<uint32_t, uint32_t> type, but it doesn’t have begin() and end() methods, so the following line of code is illegal:

std::map<uint32_t, uint32_t>::const_iterator it = x.getTheMap().begin();

The user is now prevented from using the interface incorrectly and the only way he can use it is the correct one, this one:

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();

Wow!

Advertisements

2 thoughts on “Don’t trust the temporaries (C++)!

  1. Why wouldn’t you just return by reference in your example instead? getTheMap returns by reference the map, in which case the begin/end refer to x’s map. Sorry if it’s a silly question 🙂

    1. Returning by reference makes a lot of sense and it is perfectly reasonable with the code of the example :). A particular situation in which you might not want to return by reference is when you have to return a shared_ptr in a multi-threading environment (that is the real-life situation that inspired me). In this case, the proxy wrapper may be really useful! Many thanks for asking this :)!

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s