-
Notifications
You must be signed in to change notification settings - Fork 96
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
New locker mechanism and small data race fixes #1166
base: master
Are you sure you want to change the base?
Conversation
4d08728
to
7478704
Compare
cd7d4ed
to
725f1a0
Compare
1dfc11e
to
650367c
Compare
} | ||
|
||
copyable_atomic_integral& operator=(copyable_atomic_integral other) { | ||
*this = other.load(); |
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 looks like risky. Does it potentially lead to recursive invocation of assign operator?
I suppose, will be better to rewrite
if (this != &other) {
this->store(other.load());
}
return *this;
Correct me, if I'm wrong.
std::atomic<T>(other.load()) { | ||
} | ||
|
||
copyable_atomic_integral& operator=(copyable_atomic_integral other) { |
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.
Maybe, will be better to pass by reference?
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.
No, it is intentional. It handles self assignment problem properly: currently in 56 line will be used constructor number 2. In case of your suggestions there will be problem with self assignment because of another order of function (constructor in this case) overloading.
@@ -177,11 +178,11 @@ class CompilerCore { | |||
} | |||
|
|||
void set_functions_txt_parsed() { | |||
is_functions_txt_parsed = true; | |||
is_functions_txt_parsed.store(true, std::memory_order_seq_cst); |
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.
Why do we use certain memory order option?
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 used for synchronization, where are some while(!get_functions_txt_parsed())
loops. I'm not sure that it isn't synchronized with other atomic variables.
I have tried to run transpiler with TSAN on simple program and found lots of data races. This PR fixes some of them, but not all.