patterncppMinor
std::once_flag and std::call_once implementation
Viewed 0 times
stdonce_flagandimplementationcall_once
Problem
I'm implementing
(notice: I'm aware that stuff starting with
However, the
```
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
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
-
This is begging for an enum:
Old-style enums convert implicitly to integer, so you can use one of those:
-
Trim down your comments a little bit. Some are completely pointless, like
-
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
-
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 calledOld-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 calledenum 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.