# LockedValue implementation

I am working on class for simple sharing values between threads without race-conditions, which can't be a std::atomic (this means, no POD and no trivially-copyable types). The initial intention was to lock a std::function against writing while reading from an other thread.

The class itself is simple; just lock a mutex at every sensitive part of the class, but I am not totally sure if it's correct or not. I think about to lock the mutex in the dtor, but I don't know if it's good style or not.

#pragma once

template <class T>
class LockedValue
{
private:
using Lock = std::scoped_lock<std::mutex>;

public:
LockedValue() = default;

LockedValue(const LockedValue& _other) :
{}

LockedValue(LockedValue& _other) :
LockedValue(static_cast<const LockedValue&>(_other))
{}

LockedValue(LockedValue&& _other) :
m_Value(_other._takeValue())
{}

template <class... Args>
LockedValue(Args&&... _args) :
m_Value(std::forward<Args>(_args)...)
{}

LockedValue& operator =(const LockedValue& _other)
{
store(std::move(value));
return *this;
}

LockedValue& operator =(LockedValue&& _other)
{
if (this != &_other)
store(_other._takeValue());
return *this;
}

template <class T_, typename = std::enable_if_t<std::is_convertible_v<T_, T>>>
LockedValue& operator =(T_&& _value)
{
store(std::forward<T_>(_value));
return *this;
}

T operator *() const
{
}

template <class T_>
void store(T_&& _value)
{
Lock lock(m_Mutex);
m_Value = std::forward<T_>(_value);
}

{
Lock lock(m_Mutex);
return m_Value;
}

private:
mutable std::mutex m_Mutex;
T m_Value;

T&& _takeValue()
{
Lock lock(m_Mutex);
return std::move(m_Value);
}
};


EDIT: Is it necessary to check for self-moving in move assign? I think it should be ok to remove the self check.

EDIT2: To the copy ctor related discussion. Delete the non-const& copy ctor from the class and try to use an object like this.

If you don't have c++17 change the Lock alias to std::lock_guard and change std::is_convertible_v to std::is_convertible::value

I pasted it at this online compiler with the ctor already deactivated click me

// calls the variadic ctor
LockedValue<std::string> s("test");

// this is fine; calls the usual copty ctor
auto u(static_cast<const LockedValue<std::string>&>(s));

/* results in an error because type LockedValue<std::string>&
can not be converted to std::string; this means a second call
auto t(s);

• You do need to make your code correct for self moving potentially. This does not men you need a test. The standard swaping idium works without a need for a test. – Martin York Dec 12 '17 at 21:46

You don't need both version of the copy constructor.

    LockedValue(const LockedValue& _other) :
{}

LockedValue(LockedValue& _other) :
LockedValue(static_cast<const LockedValue&>(_other))
{}


If this is a mutating copy constructor. Then just have the second one. Otherwise have the first one. Since the mutating one simple calls the const version I see no reason for the second one as a non const object will still bind to the const version.

Move operations are usually noexcept. I suppose your are potentially throwing. I would need to look up all the funtions called. You should check and if your move operations are non throwing then you need to make an appropriate comment.

    LockedValue(LockedValue&& _other) :
m_Value(_other._takeValue())
{}


A standard move is simply a swap. I suppose you would need a safe swap. Which would be doubly hard as you run into race conditions. But there is always std::lock to help you with that (But its an extra step).

    LockedValue& operator =(LockedValue&& _other)
{
if (this != &_other)
store(_other._takeValue());
return *this;
}


Thay's nice. I think I learned a new thing for the day.

    template <class T_, typename = std::enable_if_t<std::is_convertible_v<T_, T>>>
LockedValue& operator =(T_&& _value)
{
store(std::forward<T_>(_value));
return *this;
}


And a correct usage of the mutable keyword.

    mutable std::mutex m_Mutex;

• I have to use both the const ref and non-const ref ctor because if I just use the const ref (as usual) the templated ctor becomes the better match if I try to copy a non-const object. – DNKpp Dec 12 '17 at 22:03
• @DNK, use sfinae. Writing another constructor is not a good idea. – Incomputable Dec 12 '17 at 22:22
• @Incomputable if you have an idea for that, pls show it to me. I didn't have one, so I used this approach – DNKpp Dec 12 '17 at 22:27
• @DNK, it is straightforward std::enable_if_t. I recommend you to try yourself, as this is the simplest form of SFINAE. – Incomputable Dec 12 '17 at 22:28
• @Incomputable as you can see in the code above, I am able to use std::enable_if. The question is about the specific problem for the (copy)ctors. I wasn't able to make a correct type_trait expression to get what I need. – DNKpp Dec 12 '17 at 22:30

First of all, thanks for the feedback. I answer my post myself, because I have to describe my decisions.

First of all, here the current code.

#pragma once

template <class T>
class SynchronizedValue
{
private:
using Lock = std::scoped_lock<std::mutex>;

public:
SynchronizedValue() = default;

SynchronizedValue(const SynchronizedValue& _other) :
{}

SynchronizedValue(SynchronizedValue& _other) :
SynchronizedValue(static_cast<const SynchronizedValue&>(_other))
{}

SynchronizedValue(SynchronizedValue&& _other) :
m_Value(_other._takeValue())
{}

template <typename... Args>
SynchronizedValue(Args&&... _args) :
m_Value(std::forward<Args>(_args)...)
{}

SynchronizedValue& operator =(const SynchronizedValue& _other)
{
return *this;
}

SynchronizedValue& operator =(SynchronizedValue&& _other)
{
store(_other._takeValue());
return *this;
}

template <class U, typename = std::enable_if_t<std::is_convertible_v<U, T>>>
SynchronizedValue& operator =(U&& _value)
{
store(std::forward<U>(_value));
return *this;
}

T operator *() const
{
}

template <class U>
void store(U&& _value)
{
Lock lock(m_Mutex);
m_Value = std::forward<U>(_value);
}

{
Lock lock(m_Mutex);
return m_Value;
}

private:
mutable std::mutex m_Mutex;
T m_Value;

T _takeValue()
{
Lock lock(m_Mutex);
return std::move(m_Value);
}
};


As you can see, I renamed my class to SynchronizedValue, because I find it more descriptive than LockedValue. The behaviour of the class stays unchanged, I only did some minor changes and I want to describe why.

In every state of this class, only one mutex gets locked. Even in the move parts, I only had to lock one at the same time. This prevents me from getting into a deadlock. The real important part is in the T _takeValue() function. In the last state of this class I returned a rvalue reference, which was bad, because when I really started to move the value from other, outside of the function, the mutex already was getting unlocked. This means a race-condition could occur, which is absolutely bad. To fix this, my only change here is to return a real object instead of a rvalue reference. The second important change is no code change at all. It is also closely related to the _takeValue() function. I am talking about the move assignment operator. While I had to check the self assign in the previous version, I can now just take the value and store it safely. The cleanup, which is the bad thing of a self-move, already happened inside of the _takeValue() function. Thus, it should be absolutely safe to deal with the self-moving problem.

## Why no noexcept

The lock functions of a std::mutex might throw, this means I am not able to make a guarantee for exception safety, unless I would write the whole function inside of a try/catch block, which is never a good idea to deal with exceptions. It is not a mistake to declare a move-assign not noexcept; it just makes some optimizations for some std classes impossible.

## why a const& and non-const& copy ctor

That's a good question. While I know, that usually a const& copy ctor is enough, unless you have to modify the original object, it is here required to make the templated ctor possible. If I delete the non-const version of the copy ctor, everything works fine, unless I try to copy from a non-const SynchronizedValue object. A conversion from & to const& is required; and a templated overloading always has a closer match than an overloading which requires a conversation; even when it's just the const to a reference. I played a little bit around with SFINAE, but I got no nice solution for that (there might be a solution; but I wasn't able to make it work), thus I used this easy way to deal with this issue. The problem is, I can't SFINAE the copy ctor, this is the wrong way. I have to remove the templated ctor from the match list when I want to copy; and that's the real issue here. The idea, which someone came up with, was to try this one:

template <typename... Args, typename = std::enable_if_t<sizeof...(Args) != 1 || (!std::is_same_v<std:.decay_t<Args>, LockedValue> && ...)>>


but like I said, I wasn't able to get this work.