Why copy constructor, operator= and reference in this class have compile issue

Question

I tried to search for solution but found nothing. If this is a duplicated question, I apologize in advance.

I have a class like this:

class sync_lock {
    public:
        enum lock_status { free, locked };
        pthread_cond_t cond;
        pthread_mutex_t mtx;
        lock_status status;

        sync_lock() : status(free) {
            pthread_cond_init( &cond, NULL );
            pthread_mutex_init( &mtx, NULL );
        }

        sync_lock(const sync_lock & param) {
            cond = param.cond;
            mtx = param.mtx;
            status = param.status;
        }

        sync_lock & operator=(const sync_lock & param) {
            if (this == &param)
                return *this;
            cond = param.cond;
            mtx = param.mtx;
            status = param.status;
            return *this;
        }

        ~sync_lock() {
            pthread_cond_destroy( &cond );
            pthread_mutex_destroy( &mtx );
        }
};

And in the driver code, I have this:

line 29: sync_lock lock();
line 30: lock_map[lid] = lock;

Another place has this:

line 33: sync_lock & lock = lock_map[lid];

When I compile, I got this:

lock_server.cc: In member function ‘lock_protocol::status lock_server::acquire(int, lock_protocol::lockid_t, int&)’:
lock_server.cc:30:25: error: no match for ‘operator=’ in ‘((lock_server*)this)->lock_server::lock_map.std::map<_Key, _Tp, _Compare, _Alloc>::operator[] [with _Key = long long unsigned int, _Tp = sync_lock, _Compare = std::less<long long unsigned int>, _Alloc = std::allocator<std::pair<const long long unsigned int, sync_lock> >, std::map<_Key, _Tp, _Compare, _Alloc>::mapped_type = sync_lock, std::map<_Key, _Tp, _Compare, _Alloc>::key_type = long long unsigned int]((*(const key_type*)(& lid))) = lock’
lock_server.cc:30:25: note: candidate is:
lock_server.h:33:21: note: sync_lock& sync_lock::operator=(const sync_lock&)
lock_server.h:33:21: note:   no known conversion for argument 1 from ‘sync_lock()’ to ‘const sync_lock&’
make: *** [lock_server.o] Error 1

I have no idea what caused this error. Any thoughts? Thanks in advance!

Answer 1

This line:

sync_lock lock();

Does not do what you think it does. You are expecting it to declare a variable named lock of type sync_lock that is default-constructed. It actually declares a parameter-less function named lock that returns a sync_lock instance. The reason for that is known as most vexing parse.

Thus, when you then assign lock to lock_map[lid] without parenthesis on lock, you are actually assigning a function pointer, not a sync_lock instance. That is why you are getting a conversion error instead of invoking the = operator.

The fix is to simply drop the parenthesis on the variable declaration:

sync_lock lock;

Or, get rid of the variable altogether:

lock_map[lid] = sync_lock();

Answer 2

I was about to write a comment but it became too long.

This kind of classes should not be copyable.

Lets take a look to the destructor:

~sync_lock() {
    pthread_cond_destroy( &cond );
    pthread_mutex_destroy( &mtx );
}

It will destroy the mutex and the condition, invalidating any copy of the object you have.

This line:

lock_map[lid] = sync_lock();

Will do the following:

1. Create a temporary object (indexing operator)
2. Create another temporary object (call to sync_lockl())
3. Call assignment operator in the first object (point (1))
4. Destroy the object created in (2)

The consequences of this procedure are disastrous:

1. The mutex and the condition initialized in point (1) will never get destroyed because they were overwritten by the assignment operator

2. If the code had the following format:

    {
        sync_lock lock; // line 29
        lock_map[lid] = lock; // line 30
    } // line 31

The mutex and condition created in line 29 will be destroyed after line 31, when lock's destructor gets called.

Si, my advice, never make classes having this kind of resources copyable, specially when dealing with synchronization objects. If you want to put them in a container, fine, but use a pointer.

With C++11 you can make them movable, that will solve most of the problems of the current solution, but C++ also has std::mutex and std::condition so maybe you can rely on them instead of pthread.

Update: Based on the std::map implementation in GCC 4.7 (optimizations aside)

  mapped_type&
  operator[](const key_type& __k)
  {
// concept requirements
__glibcxx_function_requires(_DefaultConstructibleConcept<mapped_type>)

iterator __i = lower_bound(__k);
// __i->first is greater than or equivalent to __k.
if (__i == end() || key_comp()(__k, (*__i).first))
      // inserts new object
      __i = insert(__i, value_type(__k, mapped_type()));
return (*__i).second;
  }