Replace ink_atomic with std::atomic #2633
Conversation
zwoop
left a comment
There was a problem hiding this comment.
You're the best, really you should come to Tokyo to beat the QUIC code into submission too!
cd2f367 to
8ee83df
Compare
|
Oh no! build failure on Linux!!! |
|
We might need to add This headers should exist for gcc 4.9 or better |
8ee83df to
6b30a38
Compare
|
If it's just plugins written in C that are the problem, I say put the old |
4f1f571 to
6a3b324
Compare
| TSDebug(PLUGIN_NAME, "ref_count is %d", _ref_count); | ||
| if (1 >= ink_atomic_decrement(&_ref_count, 1)) { | ||
| TSDebug(PLUGIN_NAME, "ref_count is %d", (int)_ref_count); | ||
| if (1 >= _ref_count--) { |
There was a problem hiding this comment.
Thought this might be a problems, but it isn't.
There was a problem hiding this comment.
Which part? I am changing that to a C++ static_cast for @SolidWallOfCode.
|
[approve ci autest] |
6a3b324 to
9dd8532
Compare
jpeach
left a comment
There was a problem hiding this comment.
@PSUdaemon IMHO we should always use fetch_add and fetch_sub and never use the operators. The member functions clearly indicate the semantics to the reader and are easy to search for and find usages of. They also make it easier later to alter the memory consistency argument.
|
@jpeach if I go down that path for the inc/dec should I also do it for load/store? Basically every use of a std::atomic will require some modification. To me this kinda defeats the purpose of std::atomic. Just calling a method instead of wrapping it in a function. |
9dd8532 to
b7ac1ae
Compare
b7ac1ae to
a717f0d
Compare
|
I know there are a ton of failures right now, but I wanted @jpeach to look at what I meant with the load/store. Here is an example. |
|
@PSUdaemon I think that atomic ops deserve to be made very explicit in the code, even at the cost of extra typing ;) |
|
This PR hasn't been updated in more then 2 months and I am closing out PRs that haven't had any updates. Please feel free to reopen if want to update it. |
We should remove ink_atomic and replace it with std::atomic.