std::lock_guard should be movable

[v.lohre...@googlemail.com]

Hi,

it is really a pity that std::lock_guard is not movable. This prevent it from being used in a modern c++ style:

std::mutex mymutex;

auto lock = std::make_lock_guard(mymutex);

instead of:

std::mutex mymutex;

std::lock_guard<std::mutex> lock(mymutex);

Arguments:

std::unique_lock is movable, too. You can easily role your own movable lock_guard but the standard should provide a good default option not the to be replaced option.

Best Regards,

 Vasco

[Anthony Williams]

lock_guard is designed to be a special purpose class with minimal
overhead. That means it always owns the mutex, and doesn't have to check
to see if it owns it before unlocking. Making it movable would mean that
it didn't always own the mutex.

unique_lock *is* the movable version of lock_guard --- the rest of the
interface is just a natural consequence of a lock owner that may or may
not own the lock.

A make_lock_guard function doesn't even save you typing: your two lines
above are the same length.

Anthony
--
Author of C++ Concurrency in Action http://www.stdthread.co.uk/book/
just::thread C++11 thread library http://www.stdthread.co.uk
Just Software Solutions Ltd http://www.justsoftwaresolutions.co.uk
15 Carrallack Mews, St Just, Cornwall, TR19 7UL, UK. Company No. 5478976

[v.lohre...@googlemail.com]

Am Mittwoch, 19. März 2014 16:59:52 UTC+1 schrieb Anthony Williams:

lock_guard is designed to be a special purpose class with minimal
overhead. That means it always owns the mutex, and doesn't have to check
to see if it owns it before unlocking. Making it movable would mean that
it didn't always own the mutex.

unique_lock *is* the movable version of lock_guard --- the rest of the
interface is just a natural consequence of a lock owner that may or may
not own the lock.

I was more thinking of something like:

template <typename T> class lock_guard

{

  T* Mutex_;

  lock_guard_() = delete;

  lock_guard_(const lock_guard_&) = delete;

  lock_guard_& operator=(const lock_guard_&) = delete;

public:

  lock_guard_(T& mutex) : Mutex_(&mutex)

  {

    Mutex_->lock();

  }

  ~lock_guard_()

  {

    if(Mutex_!=nullptr)

    {

      Mutex_->unlock();

    }

  }

  lock_guard_(lock_guard_&& guard)

  {

    Mutex_ = guard.Mutex_;

    guard.Mutex_ = nullptr;

  }

};

It is movable and clearly owns the lock (or is just the empty shell being moved from and we don't care about) and I would still consider it lightweight (one additional branch). I prefer lock_guard over unique_lock exactly out of the simplicity reasons but I see no reason to have it non-movable which only degrades the usability of it.

 

A make_lock_guard function doesn't even save you typing: your two lines
above are the same length.

It is not about line count :-) 

auto locked = std::make_lock_guard(mutex);

is way better readable and behaves more nicely (refactoring?) than

std::lock_guard<WhatMutexTypeWasItAgainHopeItWontChangeAnyTimeSoon> locked(mutex);

 

[xavi]

The compiler will always be able to optimize the additional branch away, so there is no performance degradation. Still, I don't see the advantages over unique_lock, which can always be used as a drop-in replacement for what you propose, with exactly the same performance (assuming a minimum of optimization). 

 

A make_lock_guard function doesn't even save you typing: your two lines
above are the same length.

It is not about line count :-) 

auto locked = std::make_lock_guard(mutex);

is way better readable and behaves more nicely (refactoring?) than

std::lock_guard<WhatMutexTypeWasItAgainHopeItWontChangeAnyTimeSoon> locked(mutex);

 

--

---
You received this message because you are subscribed to the Google Groups "ISO C++ Standard - Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to std-discussio...@isocpp.org.
To post to this group, send email to std-dis...@isocpp.org.
Visit this group at http://groups.google.com/a/isocpp.org/group/std-discussion/.

[Nevin Liber]

On 19 March 2014 16:37, <v.lohre...@googlemail.com> wrote:

It is movable and clearly owns the lock (or is just the empty shell being moved from and we don't care about) and I would still consider it lightweight (one additional branch). I prefer lock_guard over unique_lock exactly out of the simplicity reasons but I see no reason to have it non-movable which only degrades the usability of it.

I'd much rather have the constructor solution in n3602 than making it moveable for the sole purpose of writing make_lock_guard.

 

std::lock_guard<WhatMutexTypeWasItAgainHopeItWontChangeAnyTimeSoon> locked(mutex);

That problem is easy to solve:

std::lock_guard<decltype(mutex)> locked(mutex);

It is the violation of DRY which needs a solution.

--
 Nevin ":-)" Liber  <mailto:ne...@eviloverlord.com>  (847) 691-1404

[v.lohre...@googlemail.com]

Am Mittwoch, 19. März 2014 23:00:34 UTC+1 schrieb Nevin ":-)" Liber:

On 19 March 2014 16:37, <v.lohre...@googlemail.com> wrote:

It is movable and clearly owns the lock (or is just the empty shell being moved from and we don't care about) and I would still consider it lightweight (one additional branch). I prefer lock_guard over unique_lock exactly out of the simplicity reasons but I see no reason to have it non-movable which only degrades the usability of it.

I'd much rather have the constructor solution in n3602 than making it moveable for the sole purpose of writing make_lock_guard.

Ok, I would gladly take n3602 if it would become available in the near future :-) If not, please make lock_guard movable.

 

 

std::lock_guard<WhatMutexTypeWasItAgainHopeItWontChangeAnyTimeSoon> locked(mutex);

That problem is easy to solve:

std::lock_guard<decltype(mutex)> locked(mutex);

It is the violation of DRY which needs a solution.

I guess we can both agree this is no real solution but making it slightly better in another ugly way (->DRY, yeah, another acronym I didn't know :-) ). But I knew this suggestion would been made by someone :-)

[Anthony Williams]

This is what the constructor/destructor/move constructor of unique_lock
look like. The only reason for lock_guard's existence is to avoid the
possibility of the "empty" state. This avoids potential problems due to
not knowing the state of the lock, and avoids the check in the
destructor (which cannot always be optimized out).

> A make_lock_guard function doesn't even save you typing: your two lines
> above are the same length.
>
>
> It is not about line count :-)
>
> auto locked = std::make_lock_guard(mutex);
>
> is way better readable and behaves more nicely (refactoring?) than
>
> std::lock_guard<WhatMutexTypeWasItAgainHopeItWontChangeAnyTimeSoon>
> locked(mutex);

How about this then:

class generic_lock_guard{
struct base_unlocker{
virtual void unlock(void*)=0;
};

template<typename MutexType>
struct specific_unlocker:base_unlocker{
void unlock(void* m){
static_cast<MutexType*>(m)->unlock();
}
};

void* mutex;
base_unlocker& unlocker;
alignas(specific_unlocker<std::mutex>)
char buffer[sizeof(specific_unlocker<std::mutex>)];

generic_lock_guard(generic_lock_guard const&)=delete;
generic_lock_guard& operator=(generic_lock_guard const&)=delete;

public:

template<typename MutexType>
generic_lock_guard(MutexType & mutex_):
mutex(&mutex_),
unlocker(*(new(buffer) specific_unlocker<MutexType>))
{}
~generic_lock_guard()
{
unlocker.unlock(mutex);
}
};

Usage:

generic_lock_guard guard(some_mutex);

Doesn't matter what type some_mutex is, provided it has an unlock()
member. Assumption: all instances of specific_unlocker<T> have the same
size and alignment, which seems reasonable for an empty class.

[Diggory Blake]

The extra char buffer is unnecessary - just make a static templated member function which does the equivalent of "specific_unlocker.unlock()" and then store a pointer to that method with the correct template parameter in the constructor of lock_guard. The destructor then calls this function pointer, passing in "mutex".

[Ville Voutilainen]

On 20 March 2014 14:30, Diggory Blake <dig...@googlemail.com> wrote:
> The extra char buffer is unnecessary - just make a static templated member
> function which does the equivalent of "specific_unlocker.unlock()" and then
> store a pointer to that method with the correct template parameter in the
> constructor of lock_guard. The destructor then calls this function pointer,
> passing in "mutex".

Any particular reason not to use the newly-proposed scoped resource
(http://open-std.org/JTC1/SC22/WG21/docs/papers/2014/n3949.pdf)
for this?

[Anthony Williams]

On 20/03/14 12:57, Ville Voutilainen wrote:
> Any particular reason not to use the newly-proposed scoped resource
> (http://open-std.org/JTC1/SC22/WG21/docs/papers/2014/n3949.pdf)
> for this?

scoped_resource_t is templated on the deleter --- the whole point of the
generic_lock_guard is that it is not templated on the mutex type, and
needs to do type erasure.

[Anthony Williams]

On 20/03/14 12:30, Diggory Blake wrote:
> The extra char buffer is unnecessary - just make a static templated
> member function which does the equivalent of
> "specific_unlocker.unlock()" and then store a pointer to that method
> with the correct template parameter in the constructor of lock_guard.
> The destructor then calls this function pointer, passing in "mutex".

You're right. I didn't think of that. Here's the revised class:

class generic_lock_guard{
template<typename MutexType>
struct specific_unlocker{
static void unlock(void* m){
static_cast<MutexType*>(m)->unlock();
}
};

void* const mutex;
void (*const unlocker)(void*);

generic_lock_guard(generic_lock_guard const&)=delete;
generic_lock_guard& operator=(generic_lock_guard const&)=delete;

public:

template<typename MutexType>
generic_lock_guard(MutexType & mutex_):
mutex(&mutex_),

unlocker(&specific_unlocker<MutexType>::unlock)
{}
~generic_lock_guard()
{
unlocker(mutex);
}
};