[SofaKernel] Remove usage of helper::system::atomic<int> (replaced by STL's) #1035
Conversation
fredroy
added
the
pr: status wip
fredroy
force-pushed
the
remove_system_atomicint
branch
from
88419b4 to
4c77c72
Compare
fredroy
changed the title
|
[ci-build][with-all-scenes] |
|
[ci-build][with-all-tests] |
fredroy
added
pr: status to review
| } | ||
| while(!array[writeIdx].compare_exchange_strong(refInt, item) ); | ||
| //while(array[writeIdx].compare_and_swap(-1, item) != -1) |
There was a problem hiding this comment.
Keeping the old line on purpose?
There was a problem hiding this comment.
Yes, I am not so sure that what I have done what the correct thing to do; so I kept the old line to check the previous behavior....
| } | ||
| while(item == -1); | ||
| //while((item = array[readIdx]) == -1) |
There was a problem hiding this comment.
Keeping the old line on purpose?
There was a problem hiding this comment.
Just a quick remark:
In general I always prefer to explicitly call the load/store functions for read/write atomic operations like this:
do
{
item.exchange(array[readIdx].load() );
}
while(item.load() == -1);
It helps to quickly understand or remind the developers that it's an atomic operation and it allows to set the memory order model to do some optimization.
But in this case the memory order is not specified (default value) so explicitly call the load/store function call is only to make clear that there is an atomic read/write operation.
I see that explicitly call the load/store functions was not previously used so I don't know if it worths to check all the atomic operations.
hugtalbot
added
pr: status ready
|
Thanks mr roy |
guparan
removed
pr: status to review
helper::system::atomic was introduced long time ago (before std::atomic I guess).
This code is compiling only for i386 and x86_64 code with gcc/clang (and msvc)
Now that std::atomic is here (and we are using c+>11), I guess it is time to use std::atomic where we can.
Those modifications mostly appeared in multithreading-related code, so it would be good that @fspadoni could take a look
This PR:
Reviewers will merge only if all these checks are true.