Object changing after storing and retrieving from unordered_map

Question

Consider the following code. I want to use mutex_by_name() to create and retrieve mutexes. The lock is not a real lock, but should do its job with a one second gap.

Expected output is that m4.lock() fails aka prints lock FAILED because _locked is already set to true. But it does lock. I'm new to C++ and pretty sure I'm missing something obvious. Can you please explain how to implement that correctly.

#include <iostream>
#include <string>
#include <unordered_map>
#include <unistd.h>

class Mutex {
private:
    int _id;
    bool _locked = false;
    void status(std::string s) {
        std::cout << _id << " " << name << " " << s << " " << std::endl;
    }
public:
    const std::string name;
    Mutex(std::string name): name(name) {
        static int id = 0;
        _id = id++;
        status("created");
    }
    Mutex(const Mutex& m): _id(m._id), _locked(m._locked), name(m.name) {
        status("copy-constructed");
    }
    Mutex(Mutex&& m) = delete;
    void operator=(Mutex&) = delete;
    ~Mutex() {
        status("deleted");
    }
    void lock() {
        // YES, THIS IS NOT A REAL AND SAFE LOCK
        if (!_locked) {
            _locked = true;
            status("locked");
        } else {
            status("lock FAILED");
        }
    }
};

std::unordered_map<std::string, Mutex> mutexe;

Mutex& mutex_by_name(std::string name) {
    mutexe.emplace(name, Mutex(name));
    auto found = mutexe.find(name);
    return found->second;
}


using namespace std;

int main() {
    cout << "# 1" << endl;
    Mutex m1 = mutex_by_name("hello");
    m1.lock();
    sleep(1);

    cout << "# 2" << endl;
    Mutex m4 = mutex_by_name("hello");
    m4.lock();
    sleep(1);
}

score 2 · Accepted Answer · answered Aug 29 '15 at 20:52

2

You have to problems. First of all, you're not declaring m1 and m4 as references, and they shall be so.

Secondly, code style :).

So, this shall solve it:

Mutex &m1 = mutex_by_name("hello");

//...

Mutex &m4 = mutex_by_name("hello");

answered Aug 29 '15 at 20:52

3442

8,248
2
19
41

So what's wrong about my code style? :D (emplace does not *replace* but you deleted that part anyway). Why is the compiler not complaining about assigning a Mutex& to a Mutex? What happens actually in my code? – CrypticDots Aug 29 '15 at 20:57
@valyron: Actually, use the coding style that you prefer best, that was just a joke in reference to some.... ahem... [ugly coding "standards"](https://www.gnu.org/prep/standards/). It's perfectly okay to assign a `Mutex&` to a `Mutex`. What happens is that the `Mutex&` will become the first argument to the constructor call `Mutex(const Mutex&)` for the `Mutex`. – 3442 Aug 29 '15 at 20:59
Ah those guidelines. Well, I like mine. But regarding the copy issue, I try to copy the state of `_locked` in my copy constructor. Why is it not working? – CrypticDots Aug 29 '15 at 21:04
@valyron copying of *_locked* works perfectly! But when you update it, it updates the locally coppied version, not the one in the `mutexe`. And regarding the *Copy vs. Assignment*, take a look http://stackoverflow.com/a/1051468/5218277 – Alex Lop. Aug 29 '15 at 21:11
I see. Is there a way to delete the copy constructor while still using the map? If I delete it, it won't compile. – CrypticDots Aug 29 '15 at 21:21
@valyron: We need full code examples, and you're not going the right way. ***Never delete the copy/move constructor!***. – 3442 Aug 29 '15 at 21:27
The code above is pretty much the "full code". But I'm already very satisfied with what I've found out with your answers. I think in my case I would have been enough to not implement the copy constructor and leave it the default one? – CrypticDots Aug 29 '15 at 21:39
@valyron: Maybe you're right, maybe you should use `Mutex(const Mutex&) = default`... – 3442 Aug 29 '15 at 21:41

score 1 · Answer 2 · answered Aug 29 '15 at 20:40

1

In main you need to make m1 and m4 references (Mutex &m1). Right now they are copies and thus aren't updating the value in the unordered map.

answered Aug 29 '15 at 20:40

Michael Albers

3,721
3
21
32

It solves the problem. But I don't really understand why. Where is the copy happening? Why is the compiler not complaining about assigning a Mutex& to a Mutex? – CrypticDots Aug 29 '15 at 20:50
@valyron: A `Mutex&` may be assigned to a `Mutex`, in which case a *copy* occurs. – 3442 Aug 29 '15 at 20:53
In your line " Mutex m1 = mutex_by_name("hello");" you're telling the compiler that m1 is its own object. In this case the reference is treated like a normal object and not the pointer that it is under the hood. m1 is constructed using the copy constructor. The compiler isn't complaining because it's perfectly legal. You're saying "I want to create a new object, m1, with the contents of the reference returned from mutex_by_name"? Make sense? – Michael Albers Aug 29 '15 at 20:56
Okay I understand that, but in my copy constructor, `_locked` is copied to the new instance, so why does that not work? – CrypticDots Aug 29 '15 at 21:01
It's copied by value. _locked in m1, m4 and the mutex in the map all have separate memory locations. Updating one does not affect any of the others (unless you make them references). – Michael Albers Aug 29 '15 at 21:03

Object changing after storing and retrieving from unordered_map

2 Answers2