Should I store a reference to map in a member variable?

Question

I have the following classes:

class State {
    std::string name;
    int id;
};

class Device {
private:
    int stateID;
public:
    const std::map<int, State> possibleStates;
    Device(std::map<int, State> states) : possibleStates(states) {}
};

I would like to be as memory efficient as possible. My question is, does it make any difference if I store the map in the Device class as a reference (e.g. const std::map<int, State>& possibleStates)? If I keep the code as is, would a copy of the map be created every time I called the Device constructor?

Answer 1

Your code as-is will make a copy of your map (including copies of the states held in the values). Having a reference will take less memory, but something else will be responsible for the map. Will all devices share the same map of possible states?

It looks more like you want an enumeration to hold the state, and a map to provide a linking between state and name.

class Device {
public:
    enum State {
        init,
        state_2,
        state_c,
    };
    static const std::unordered_map<State, std::string> state_names;

    Device() : stateID(State::init) {}

private:
    State stateID;
};

The static variable needs initialization, though. This is usually done in a .cpp file, not a .h file, since you only want it initialized in one compilation unit.

const std::unordered_map<Device::State, std::string> Device::state_names {
    {State::init, "initialize"},
    {State::state_2, "second state"},
    {State::state_c, "another state"}
};

All together it looks something like this:

#include <unordered_map>
#include <string>
#include <iostream>

class Device {
public:
    enum State {
        init,
        state_2,
        state_c,
    };
    static const std::unordered_map<State, std::string> state_names;

    Device() : stateID(State::init) {}
    Device(State s) : stateID(s) {}

    const std::string& current_state() const {
        return state_names.at(stateID);
    }

private:
    State stateID;
};

const std::unordered_map<Device::State, std::string> Device::state_names {
    {State::init, "initialize"},
    {State::state_2, "second state"},
    {State::state_c, "another state"}
};

int main(int /* argc */, char** /* argv */) {
    Device d1;
    Device d2(Device::State::state_c);

    std::cout << "Device 1 state: " << d1.current_state() << '\n';
    std::cout << "Device 2 state: " << d2.current_state() << '\n';
}

Output:

Device 1 state: initialize
Device 2 state: another state

Godbolt

Now you only have a compile-time set of the possible states, a single immutable mapping of the states to their names, and it's all accessible by any class.

Answer 2

A reference member is allowed, but a dangerous thing. It needs to refer to another object outside the class object, where some other code makes sure the object referred to is certain to exist for as long as the class member might be used again.

And if you changed just the member type and had:

class Device {
private:
    int stateID;
public:
    const std::map<int, State>& possibleStates;
    Device(std::map<int, State> states) : possibleStates(states) {}
};

then this would almost certainly be wrong. The reference would bind to the parameter states of the constructor, and the lifetime of states ends as soon as the constructor body finishes, leaving possibleStates a dangling reference!

If you also changed the constructor parameter type, then it becomes sort of usable...

class Device {
private:
    int stateID;
public:
    const std::map<int, State>& possibleStates;
    Device(const std::map<int, State>& states) : possibleStates(states) {}
};

... but now every bit of code which creates a Device object needs to be careful and make sure the map argument passed in is one which will exist for as long as it's needed. Users of the class may not be expecting this, and even if it's known, it's too easy to accidentally get wrong, like by creating a Device from an unnamed temporary std::map object, which will very quickly be destroyed.

The real solution to avoid unnecessary copies is:

#include <utility>

class Device {
private:
    int stateID;
public:
    const std::map<int, State> possibleStates;
    Device(std::map<int, State> states) : possibleStates(std::move(states)) {}
};

The std::move lets the map constructor know that this code doesn't care about keeping a consistent value in the states object. So the move constructor for std::map<int, State> is permitted to "steal" allocated memory from states to create possibleStates, rather than duplicating all the memory for map internal details and string contents. This makes sense since states is local to the constructor definition and about to be destroyed, and nothing else even names the parameter variable again afterward.

Keeping the parameter type in Device(std::map<int, State>); as an object type rather than a reference type also allows the code which creates a Device to determine whether it should create that parameter object by copy, or by move, or using some other map constructor entirely.