It is not clear to me why changing to fetch_add provides a solution here. op++ also calls fetch_add albeit with different memory ordering. Consequently, you cannot avoid the problem you are trying to solve. To be able to, you would need to perform all the involved operations atomically by protecting concurrent access in a critical section.

The problem was "a series of atomic operations of LOG_OCCURRENCES_MOD_N, but in whole, they are not atomic, which unexpected". Here only the LOG_OCCURRENCES is static, the only place we need to protect. Specifically, we only do one atomic operation fetch_add on LOG_OCCURRENCES.

My question is: in what way do the changes fix the problem? The operations are still not atomic as a whole.

The operations are still not atomic as a whole.

Yes. But they don't need to be. There is only one operation that needs to be atomic, access to the static variable LOG_OCCURRENCES.

in what way do the changes fix the problem?

For every use of this macro (under different threads), a uniqueLOG_OCCURRENCES value is assigned, incremented by 1. We use a local variable LOG_OCCURRENCES_MOD_N to get the reminder of % n. By now, there is no need for protection or being atmoic. It can be written like

int LOG_OCCURRENCES_MOD_N = LOG_OCCURRENCES.fetch_add(1, std::memory_order_relaxed);
if (LOG_OCCURRENCES_MOD_N % n == 1)

They are the same.

You cannot do that. Once LOG_OCCURRENCES overflows you will hit undefined behavior.

Also, the change from op++ to fetch_add is not clear to me. Why is this necessary? If you need the previous value, you can use the post increment.

The main idea is to promise every call can have the exact value in the if condition statements to represent if it should be logged. When the static atomic variable is fetch_add to a local variable, the state is recorded. You'll never get the same value.

Once LOG_OCCURRENCES overflows you will hit undefined behavior.

You're right. This may need more consideration.

the change from op++ to fetch_add is not clear to me. Why is this necessary?

int val = op++; // not atomic, there are two operations. self increment and assignment. ++op is the same
int val = op.fetch_add(1) // atomic

That's not correct. With post increment you will get the previous value since it essentially calls fetch_add as well.

Please refer to the documentation of std::atomic::op++ before we continue the discussion. Thanks.

Sorry, It's my bad. You're right. I agree there is no need to use fetch_add. ++ is enough.

Once LOG_OCCURRENCES overflows you will hit undefined behavior.

Use atomic_uint instead. Unsigned integer overflow is not a UB. Will this help?