Re: [boost] [release/atomic] request permission to merge 86144

Subject: Re: [boost] [release/atomic] request permission to merge 86144
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2013-10-14 20:43:57

On 15/10/2013 13:06, Quoth Andrey Semashev:
> However, I would appreciate if you tried out the current trunk sans
> the commit 86144 to verify it works.

That could be awkward for me at present, but I'll see if I can try the 
1.55 beta build, which should have the same effect as that commit hasn't 
been merged through yet.

> No, the trunk code before 86144 had support for all 64-bit operations, both
> for 64 and 32-bit targets. In the latter case, they were implemented through
> DCAS (see atomic/detail/cas64strong.hpp).

Ah yes, I see now.  I don't have the trunk code downloaded at the moment 
so I'm looking at it through an SVN browser peephole, and it's easy to 
miss details in other files.

> Are we looking at the same code? The change to windows.hpp starts at line
> 1313, which is inside the "#if defined(_M_AMD64) || defined(_M_IA64)" block.
> The rest of the changes are also within this block. I'm looking at the current
> trunk now.

Sorry, I thought you were referring to the interlocked.hpp changes.

You're right, it looks like the windows.hpp changes have been 
misapplied, due to the changes between 1.54 and 1.55/trunk.  (They're 
not *completely* useless, as it still defines a fallback path in case 
eg. BOOST_ATOMIC_INTERLOCKED_EXCHANGE_ADD64 is not defined on x64 for 
some reason, but as currently that can't happen it's a needless redundancy.)

> That macro is not defined when interlocked.hpp is included. The change to
> interlocked.hpp is not needed because windows.hpp doesn't use the
> _InterlockedCompareExchange64 intrinsic to implement cmpxchg8b.

I noticed that, although I'm not entirely sure why it doesn't.  Wouldn't 
using the intrinsic be simpler than using the assembly code directly? 
It probably doesn't really matter in the end though, the effect should 
be the same.

> Given the above, I don't think so. I'm in favor of reverting that commit.

I agree with that now.  I was not aware of the 1.55 changes at the time 
I made the patch.