Implementing GSL synchronized_value

Question

Core Guidelines mention a type synchronized_value<T>, which supposedly pairs std::mutex with the internal value. I couldn't find any implementation both in GSL-Lite and MS GSL libraries. The SV class stores T value coupled with std::mutex. It implements pointer and dereference operators which create a temporary object of xlock which locks the mutex while alive.

Example usage is:

synchronized_value<Dummy> svDummy{}; // when Dummy is default-constructable
Foo(*svDummy);

When Foo() is called, a temporary xlock locks the mutex and keeps it until the function returns.

I implemented my class and want to have it reviewed.

I decided to delete move constructors of xlock, because it stores a reference and I don't want to deal with invalid state. Moreover I see no reason for such functionality.

To check the implementation I have written 3 tests: 2 control with std::lock_guard and std::unique_lock that explicitly lock a separate mutex. And 1 with synchronized_value. All the tests are passing.

Here is a link to my repository with the library and tests (CTest).

#pragma once

#include <mutex>
#include <type_traits>

template<typename ...>
using void_t = void;

template<typename T, typename Tuple, typename = void_t<>>
struct is_aggregate_constructable_ : std::false_type
{
};

template<typename T, typename ... Args>
struct is_aggregate_constructable_<T,
        std::tuple<Args...>,
        void_t<decltype(T{Args()...})>> : std::true_type
{
};

template<typename T, typename ... Args>
struct is_aggregate_constructable :
        is_aggregate_constructable_<T, std::tuple<Args...>>
{
};

// TODO: add timer?
template<typename T, typename Mutex = std::mutex>
class xlock
{
public:
    using value_type = std::conditional_t<std::is_const<T>::value, const T, T>;
    using mutex_type = Mutex;

    xlock(value_type &refValue, Mutex &mutex)
            : refValue_(refValue),
              uniqueLock_(mutex)
    {}

    xlock(const xlock &other) = delete;

    xlock(xlock &&other) = delete;

    xlock &operator=(const xlock &other) = delete;

    xlock &operator=(xlock &&other) = delete;

    value_type *operator->()
    { return &refValue_; }

    const value_type *operator->() const
    { return &refValue_; }

    value_type &operator*()
    { return refValue_; }

    const value_type &operator*() const
    { return refValue_; }

    operator value_type &()
    { return refValue_; }

    operator const value_type &() const
    { return refValue_; }

    ~xlock()
    {

    }

private:
    value_type &refValue_;
    std::unique_lock<mutex_type> uniqueLock_;
};

template<typename T, typename Mutex = std::mutex>
class synchronized_value
{
public:
    using value_type = T;
    using mutex_type = Mutex;
    using xlock_t = xlock<value_type, mutex_type>;
    using cxlock_t = xlock<const value_type, mutex_type>;

    template<typename =
    std::enable_if_t<std::is_default_constructible<T>::value>>
    synchronized_value()
    {};

    explicit synchronized_value(T value)
            : value_(std::forward<T>(value))
    {}
    synchronized_value(const synchronized_value &) = delete;
    synchronized_value &operator=(const synchronized_value &) = delete;

    synchronized_value(synchronized_value &&other) = delete;
    synchronized_value &operator=(synchronized_value &&other) = delete;

    xlock_t operator*()
    { return {value_, mutex_}; }

    cxlock_t operator*() const
    { return {value_, mutex_}; }

    xlock_t operator->()
    { return {value_, mutex_}; }

    cxlock_t operator->() const
    { return {value_, mutex_}; }

    value_type &value()
    { return value_; }

    const value_type& value() const
    { return value_; }

private:
    value_type value_;
    mutable mutex_type mutex_;
};

Testing functions:

// run_check.h

#pragma once

#include "synchronized_value/synchronized_value.h"

#include <iostream>
#include <thread>

struct Dummy
{
    int64_t id;
    bool b;
};

struct Wrapped
{
    synchronized_value<Dummy>& sv;
};

// false if check is successful
bool run_check(Dummy& d, int64_t assign, size_t intervals)
{
    d.id = assign;

    // std::cout << "run_check " << assign << " thread: " <<
       //       std::this_thread::get_id() << '\n';
    for (size_t i = 0; i != intervals; ++i)
        ++d.id;
    // std::cout << "end_check " << assign << " thread: " <<
       //       std::this_thread::get_id() << '\n' << std::endl;

    return bool(d.id - assign - intervals);
}

bool run_check_w(Wrapped& w, int64_t assign, size_t intervals)
{
    return run_check(*w.sv, assign, intervals);
}

template <typename Guard>
bool check_mutex(Dummy& d, std::mutex& mutex, uint64_t assign, size_t intervals)
{
    Guard guard{mutex};
    d.id = assign;

    for (size_t i = 0; i != intervals; ++i)
        ++d.id;

    return bool(d.id - assign - intervals);
}

Testing unit:

#include "run_check.h"

#include <future>
#include <random>

#include <string>
#include <iostream>

template <typename T1, typename T2>
constexpr bool is_same_v = std::is_same<T1, T2>::value;


int main(int argc, const char **argv)
{
    size_t intervals = 10000;

    if (argc == 2)
    {
        try
        {
            intervals = std::stoll(argv[1]);
        }
        catch (const std::invalid_argument &)
        {
            std::cerr << "Invalid argument for intervals: using " << intervals <<
                      " intervals fot test" << std::endl;
        }
    }

    static_assert(is_aggregate_constructable<Dummy, uint64_t, bool>(), "Invalid result");
    static_assert(is_aggregate_constructable<Dummy, int, bool>(), "Invalid result");
    static_assert(is_aggregate_constructable<Dummy, int, int>(), "Invalid result");
    static_assert(!is_aggregate_constructable<Dummy, double, int>(), "Invalid result");

    static_assert(is_same_v<typename xlock<Dummy>::value_type,
            Dummy>, "Invalid result");
    static_assert(is_same_v<typename xlock<const Dummy>::value_type,
            const Dummy>, "Invalid result");

    std::mutex mutex;
    Dummy d{1};
    {
        xlock<Dummy> xlock1{d, mutex};
        xlock1->id;
        Dummy dummy = *xlock1;
        Dummy& refDummy = *xlock1;
        const Dummy& crefDummy = *xlock1;
    }
    {
        xlock<const Dummy> xlock_const{d, mutex};
        xlock_const->id;

        Dummy dummy = *xlock_const;
        // Dummy& refDummy = *xlock_const;
        const Dummy& crefDummy = *xlock_const;
    }
    {
        const xlock<const Dummy> xlock_const{d, mutex};
        xlock_const->id;
    }
    {
        synchronized_value<Dummy> synchValue{d};
        synchValue->id;
        Dummy dummy = *synchValue;
        Dummy& refDummy = *synchValue;
        const Dummy& crefDummy = *synchValue;
    }
    {
        const synchronized_value<Dummy> synchValueConst2{d};
        synchValueConst2->id;
        Dummy dummy = *synchValueConst2;
        // Dummy& refDummy = *synchValueConst2;
        const Dummy& crefDummy = *synchValueConst2;
    }

    synchronized_value<Dummy> dummy{{16}};

    bool first_check = run_check(*dummy, 42, 1000);

    std::random_device rd;
    std::mt19937 rng(rd());
    std::uniform_int_distribution<int64_t> dist(-69800, 385697);

    std::vector<Wrapped> input(1000, {dummy});
    std::vector<std::future<bool>> results;

    for (size_t i = 0; i != 1000; ++i)
    {
        results.push_back(std::async(std::launch::async,
                &run_check_w,
                std::ref(input[i]),
                dist(rng),
                intervals));
    }

    int result = 0;
    for (size_t i = 0; i != 1000; ++i)
        result += (int)results[i].get();

    return result;
}

Control tests you can find at my repo.

How it can be improved? What may have I missed?

---

I updated this question. I found out that initial test was incorrect, but I have fixed it. Now everything works

Answer 1

#pragma once

• Use #ifndef guards instead of #pragma once to be standard conforming.

#include <mutex>
#include <type_traits>

• Missing #include <tuple>.

template<typename ...>
using void_t = void;

// more code 

• Don't pollute the global namespace. Wrap your code into its own namespace.

template<typename T, typename Tuple, typename = void_t<>>
struct is_aggregate_constructable_ : std::false_type
{
};

• C++ uses constructible over constructable.

template<typename T, typename ... Args>
struct is_aggregate_constructable_<T, std::tuple<Args...>, void_t<decltype(T{Args()...})>> : std::true_type
{
};

• Is your trait really testing aggregate constructibility? It seems you are testing for list initialization. Consider T = std::vector<U>. The std::vector::vector(std::initializer_list) constructor allows list inititalization, satisfying your type trait, yet std::vector isn't an aggregate.

template<typename T, typename Mutex = std::mutex>
class xlock
{ 
    // code...
};

• xlock is a confusing name. It's a lock guard that provides scoped access to the wrapped object inside the synchronized_value instance. N4033 refers to this type of guard as an update_guard. Boost uses a similar type called strict_lock_ptr.

    xlock(const xlock &other) = delete;
    xlock(xlock &&other) = delete;
    xlock &operator=(const xlock &other) = delete;
    xlock &operator=(xlock &&other) = delete;
    ~xlock()
    {

    }

• Prefer =default instead of providing an empty destructor body. By providing a destructor ({}), your object is no longer trivially destructible.

    template<typename =
    std::enable_if_t<std::is_default_constructible<T>::value>>
    synchronized_value()
    {};

    explicit synchronized_value(T value)
            : value_(std::forward<T>(value))
    {}

• What about the case where T is not default constructible or copy constructible? When defining a inplace constructor, you'll need to ensure the argument passed isn't the same as synchronized_value, otherwise it'll act as a copy constructor (which you didn't want).

    synchronized_value(const synchronized_value &) = delete;
    synchronized_value &operator=(const synchronized_value &) = delete;
    synchronized_value(synchronized_value &&other) = delete;
    synchronized_value &operator=(synchronized_value &&other) = delete;

• Prefer to explicitly define the five special member functions if any of them are user-defined. ~synchronized_value() = default;
• Try to be consistent in the ordering of your definitions. You interleave the copy and move operations here. In xlock, you group them by operation type.

    value_type &value()
    { return value_; }

    const value_type& value() const
    { return value_; }

• synchronized_value shouldn't provide direct access to the underlying value. Nothing is protecting against a data race when calling value().
• Be consistent with your types. Sometimes & is attached to the type. Sometimes it is attached to the identifier. Pick one or use an alias (e.g. reference, const_reference).