HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

std::once_flag and std::call_once implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stdonce_flagandimplementationcall_once

Problem

I'm implementing std::once_flag and std::call_once for a particular MinGW build where they are not available, using only stuff available in the rest of the standard. The required semantics would be easy to implement with something like:

struct once_flag {
private:
    std::mutex _M_mutex;
    std::atomic_bool _M_has_run;
public:
    /// Default constructor
    once_flag() : _M_has_run(false) {}

    /// Deleted copy constructor
    once_flag(const once_flag&) = delete;
    /// Deleted assignment operator
    once_flag& operator=(const once_flag&) = delete;

    template
    friend void
    call_once(once_flag& __once, _Callable&& __f, _Args&&... __args);
};

/// call_once
template
void
call_once(once_flag& __once, _Callable&& __f, _Args&&... __args)
{
    // Early exit without locking
    if(__once._M_has_run) return;
    unique_lock __l(__once._M_mutex);
    // Check again now that we locked the mutex
    if(__once._M_has_run) return;
    __f(std::forward(__args)...);
    __once._M_has_runs = true;
}


(notice: I'm aware that stuff starting with __ and _[A-Z] is reserved for the standard library implementation; the point is that this code is meant to be the standard library implementation, so it's ok)

However, the once_flag constructor is required to be constexpr to make sure its initialization is thread-safe (or at least, so it seems from this mail about boost::once_flag); this complicates the matter, since std::mutex constructor is not constexpr. I came up with this kludge:

```
struct once_flag {
private:

// Use a union to defer the initialization of the mutex to call_once
union {
std::mutex _M_mutex;
char _M_dummy[sizeof(_M_mutex)];
};
// Spinlock used for mutex initialization
std::atomic_flag _M_spinlock = ATOMIC_FLAG_INIT;
std::atomic_int _M_run_status; // 0: not initialized; 1: mutex initialized; 2: function called
public:
/// Default constructor
constexpr once_flag() noexcept : _M_r

Solution

Just a few basic suggestions on code improvement, hopefully someone else will be able to give you a few hints on the concurrency aspects of the code later.

-
Please drop the __ prefixing. I know you wanted to make this look like code from the std library, but the truth is that code in the library headers is obfuscated on purpose. Prefixing everything with two underscores makes the code look soo ugly. There's no reason to do that besides intentional obfuscation, and I'm sure you want your code to be programmer-friendly and maintainable.

-
This is begging for an enum:

std::atomic_int _M_run_status; // 0: not initialized; 1: mutex initialized; 2: function called


Old-style enums convert implicitly to integer, so you can use one of those:

enum RunState {
    NotInitialized,
    MutexInitialized,
    FunctionCalled
};


-
Trim down your comments a little bit. Some are completely pointless, like // Default constructor, others are just repeating what can be clearly figured by just reading the code: // Initialize the mutex, // Call __f...

-
I would put all that mutex initialization code inside a separate helper method, since it is fairly complicated and distracts from the whole point of call_once which is just running a callback:

call_once(once_flag& once, Callable&& func, Args&&... args)
{
    if (once.m_run_status == FunctionCalled) {
        return;
    }

    // A private member that friend call_once can access
    once.check_mutex_initialized(); 

    std::unique_lock lock(once.m_mutex);

    // Check again; we may be the *second* thread to acquire the mutex
    if (once.m_run_status == FunctionCalled) {
        return;
    }

    func(std::forward(args)...);
    once.m_run_status = FunctionCalled;
}

Code Snippets

std::atomic_int _M_run_status; // 0: not initialized; 1: mutex initialized; 2: function called
enum RunState {
    NotInitialized,
    MutexInitialized,
    FunctionCalled
};
call_once(once_flag& once, Callable&& func, Args&&... args)
{
    if (once.m_run_status == FunctionCalled) {
        return;
    }

    // A private member that friend call_once can access
    once.check_mutex_initialized(); 

    std::unique_lock<mutex> lock(once.m_mutex);

    // Check again; we may be the *second* thread to acquire the mutex
    if (once.m_run_status == FunctionCalled) {
        return;
    }

    func(std::forward<Args>(args)...);
    once.m_run_status = FunctionCalled;
}

Context

StackExchange Code Review Q#117468, answer score: 4

Revisions (0)

No revisions yet.