-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Thread-safe lazy initialization utils #84
base: master
Are you sure you want to change the base?
Conversation
Once(Once<T> const&) = delete; | ||
|
||
/// @brief Gets the current value, if it is initialized. | ||
std::optional<T const&> get() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not valid for an optional to hold a reference type. It also doesn't really make sense here, as you literally just return value
right? value
isn't an optional wrapping a reference type, so why should the return type for this (and the next one) be that way?
/// @param initializer_ Function to use for initialization on the first access. | ||
Lazy(F const& initializer_) : inner(), initializer(initializer_) {} | ||
|
||
operator T() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'm not sure how I feel about implicit conversion operators, but this is ultimately a non-issue The repeated code here bothers me a little, but making a macro for it is probably unnecessary.
#include <condition_variable> | ||
#include <mutex> | ||
|
||
constexpr std::uintptr_t UNINIT = 0x0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally adverse to having constexpr
constants, instead preferring enum
or enum class
which results in the same result but is in a more "segmented" location, though this again doesn't really matter too much.
|
||
~WaiterQueue() { | ||
// Grab the queue and swap it with the value it should have when destroyed. | ||
auto const state_and_queue = this->state_and_queue.exchange(set_state_on_destroy_to, std::memory_order_acq_rel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm usually against shadowing
constexpr std::uintptr_t RUNNING = 0x1; | ||
constexpr std::uintptr_t INIT = 0x2; | ||
|
||
constexpr std::uintptr_t STATE_MASK = 0b11; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment here would be nice to show the actual reason behind the bitmask here (and how it needs to fit with the others)
} else /* waiting */ { | ||
assert((state_and_queue & STATE_MASK) == RUNNING); | ||
|
||
while ((state_and_queue & STATE_MASK) == RUNNING) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something something do-while? :P
} | ||
//// @brief Gets the current value, if it is initialized. | ||
std::optional<T&> get() { | ||
return value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also imagine that because this is just returning the value of the optional, there is a potential for it to not yet be initialized, no? Though I don't know if that will just fallback nullopt
or be some weird garbage
No description provided.