On 01/25/18 02:32, Gavin Lambert via Boost wrote:
On 25/01/2018 00:35, Andrey Semashev wrote:
I would like to ask for the community opinion on the naming of the (op)_and_test functions that appeared in Boost.Atomic 1.66.
For clarity, I was the one who filed the issue mentioned in the OP.
My argument is the following:
* "if (x)" is true when x is an integer type that is nonzero. (And this convention is often extended to non-integer types as well, for suitable definitions of "zero".) * "atomic_flag::test_and_set" is true when the flag was previously set. * "atomic<T>::bit_test_and_set" is true when the bit was previously 1/set. * "atomic<T>::fetch_add" returns the value prior to the add, which is true if nonzero due to the first rule.
It thus seems peculiar to have "atomic<>::add_and_test" return true when the result is zero.
I can understand why this was done, as it's a natural consequence of the assembly implementation that tends to operate around a "zero flag" rather than a "nonzero flag", but it seems strongly counter-intuitive as an interface in a higher level language.
Originally, when I picked the name, CPU flags were not my motivation. There are different instructions that test the flag for being set as well as being not set, so there's no difference from this perspective. I guess, to me "test" may mean checking for something being zero or not zero depending on the context, and I found it acceptable to use this word alone for the sake of shorter function names. Remembering that the functions check the result for being zero did not seem that much of a problem and the intended use case seemed to favor that interpretation and not the opposite.
To me at least, "test" itself implies "return true if non-zero", partly as a consequence of these other things.
So "bit_test_and_set" would fundamentally mean "test if the bit is non-zero, then set it and return the result of the prior test"... which is indeed what it does.
And "add_and_test" would fundamentally mean "add this and return true if the result is non-zero"... which is *not* what the current implementation does.
So would you prefer to keep the name and change the result?