[atomic] (op)_and_test naming

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[atomic] (op)_and_test naming

Boost - Dev mailing list
Hi,

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.

As you probably know, 1.66 included a few new atomic operations, some of
which have the name (op)_and_test (e.g. add_and_test, sub_and_test,
etc.) These operations perform the (op) on the atomic value and test if
the result is zero, so that instead of this:

     if (a.fetch_sub(1) - 1 == 0)

one could write this:

     if (a.sub_and_test(1))

Also, the generated code for these operations can potentially be more
efficient.

Recently I received this request:

https://github.com/boostorg/atomic/issues/11

which basically asks to change the result of the functions to the
opposite. I can see that the current naming could be confusing,
especially given that other operations like bit_test_and_set return the
previous bit value (i.e. true if the bit was 1). That would be a
breaking change, but the new operations were announced as experimental
in the release notes, so if there is consensus that the result should
indeed be changed, I'm ready to comply.

Another solution is change (op)_and_test naming to something else that
is more clear. In that case I could deprecate the (op)_and_test variants
and provide the functionality under the new names. I would welcome
suggestions for a better naming scheme, if you feel this is the right
way. My only ask is that the new names be concise, if possible.

Lastly, if you think the current naming is fine or there is another way
to resolve the confusion, please comment as well. I would appreciate
your opinion in any case.

Thanks.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
Andrey Semashev wrote:

> My only ask is that the new names be concise, if possible.

The concisest I can come up with is sub_and_check_if_zero; anything else
either doesn't read correctly, or is still ambiguous.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
On 01/24/18 16:09, Peter Dimov via Boost wrote:
> Andrey Semashev wrote:
>
>> My only ask is that the new names be concise, if possible.
>
> The concisest I can come up with is sub_and_check_if_zero; anything else
> either doesn't read correctly, or is still ambiguous.

Maybe sub_and_test_zero then?

So, do I understand correctly that, in your opinion, the current naming
is confusing and should be changed?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
Andrey Semashev wrote:

> On 01/24/18 16:09, Peter Dimov via Boost wrote:
> > Andrey Semashev wrote:
> >
> >> My only ask is that the new names be concise, if possible.
> >
> > The concisest I can come up with is sub_and_check_if_zero; anything else
> > either doesn't read correctly, or is still ambiguous.
>
> Maybe sub_and_test_zero then?
>
> So, do I understand correctly that, in your opinion, the current naming is
> confusing and should be changed?

If we assume that it's confusing - and if it weren't you wouldn't be
starting this thread - changing the return value from false to true would do
nothing to improve matters, in my opinion. There's nothing in the name that
indicates whether it returns true or false, so it'll just confuse the other
half.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.

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.


(And I know someone's probably going to raise POSIX's test(1) as a
counter-argument, which is true when zero.  But that's because it
follows the shell's truthiness conventions, which are different from
those of C/C++.)


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
On January 24, 2018 6:32:01 PM EST, Gavin Lambert via Boost <[hidden email]> 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.

Your consistency argument seems compelling, unless there are unidentified counterexamples.

--
Rob

(Sent from my portable computation device.)

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 01/25/18 14:32, Rob Stewart via Boost wrote:

> On January 24, 2018 6:32:01 PM EST, Gavin Lambert via Boost <[hidden email]> 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.
>
> Your consistency argument seems compelling, unless there are unidentified counterexamples.

I suppose, I can add one other example supporting that pattern:
"std::bitset<N>::test".

Also, the standard includes wording such as "returns a value testable as
true" when it describes predicates that can be used with algorithms,
which means that the value can be contextually converted to bool (i.e.
true if not "zero").

It seems like the term "test" indeed implies "check if something is not
zero".

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
Andrey Semashev wrote:

> It seems like the term "test" indeed implies "check if something is not
> zero".

It doesn't. Testing a _bit_ does test whether the bit is 1. Testing a
condition does not imply that the condition "is not zero" is preferred over
everything else. The x86 `test` instruction for example sets all condition
codes, and it's up to the following conditional branch (jz, jnz, js, jns) to
determine which one is being tested. (Ironically in the `jz` case it still
tests whether a bit is 1 - the `zf` bit.)

But in any event, you can't settle an empirical question by reasoning. It's
either confusing (to the actual audience) or it isn't, and if it is,
elaborate rationalizations about why it shouldn't be do not change the
outcome. You could just avoid that whole debate by making the name less
ambiguous.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
On January 25, 2018 9:51:16 AM EST, Peter Dimov via Boost <[hidden email]> wrote:

> Andrey Semashev wrote:
>
> > It seems like the term "test" indeed implies "check if something is
> > not zero".
>
> It doesn't. Testing a _bit_ does test whether the bit is 1. Testing a
> condition does not imply that the condition "is not zero" is preferred
> over
> everything else. The x86 `test` instruction for example sets all
> condition
> codes, and it's up to the following conditional branch (jz, jnz, js,
> jns) to
> determine which one is being tested. (Ironically in the `jz` case it
> still
> tests whether a bit is 1 - the `zf` bit.)
>
> But in any event, you can't settle an empirical question by reasoning.
> It's
> either confusing (to the actual audience) or it isn't, and if it is,
> elaborate rationalizations about why it shouldn't be do not change the
> outcome. You could just avoid that whole debate by making the name
> less ambiguous.

Consistency within this domain should be sufficient. If these functions, once made consistent, are confusing, all the others using "test" will be confusing, too. All must be renamed to solve that problem.

--
Rob

(Sent from my portable computation device.)

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 26/01/2018 01:45, Andrey Semashev wrote:
> So would you prefer to keep the name and change the result?

That is my preference, yes.  Though I know it could be problematic since
it's an effectively invisible breaking change.  And while it's new,
there could still be some people using it in its current form before
1.67 is released.

Deprecating the current method and adding a new one with a new name is
the safer way to change the behaviour, but unfortunately "x_and_test"
already seems like the best name for this operation.

"add_test" could also work I guess (since existing methods like
"fetch_add" also lack a conjunction, I suppose one could argue that it
could be even more consistent that way).  And it does get rid of the
slightly awkward "and_and_test", so it's not all bad.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Rob Stewart wrote:

> Consistency within this domain should be sufficient.

Consistency and intuition are in conflict here. These function are presently
specified the way they are specified for a reason - this is what makes more
sense in the context in which they are typically used. You can use
consistency to justify a choice, but you can't magically make people not be
confused by the choice.

This is similar to the bool conversion of std::error_code, which is also
perfectly consistent and still manages to return the opposite of what a
certain fraction of the audience intuitively expects.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
On Thu, Jan 25, 2018 at 6:03 PM, Peter Dimov via Boost
<[hidden email]> wrote:
> Consistency and intuition are in conflict here. These function are presently
> specified the way they are specified for a reason - this is what makes more
> sense in the context in which they are typically used. You can use
> consistency to justify a choice, but you can't magically make people not be
> confused by the choice.
>
> This is similar to the bool conversion of std::error_code, which is also
> perfectly consistent and still manages to return the opposite of what a
> certain fraction of the audience intuitively expects.

I haven't been following this specific discussion closely, but in
general, I wish designers put a higher value on consistency than on
trying to guess what the audience's expectations are.  Consistency
leads to tools whose behavior can be reasoned about and competence
with can be achieved.  Guessing what the people want leads to
incomprehensible black boxes that the user pokes at, hoping it will
guess what they want it to do.


--
Frank

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
Frank Mori Hess wrote:

> I haven't been following this specific discussion closely, but in general,
> I wish designers put a higher value on consistency than on trying to guess
> what the audience's expectations are.  Consistency leads to tools whose
> behavior can be reasoned about and competence with can be achieved.
> Guessing what the people want leads to incomprehensible black boxes that
> the user pokes at, hoping it will guess what they want it to do.

The specific situation under discussion is as follows:

1. Andrey has provided a function x.sub_and_test(v) which returns true when
the result is zero.
2. Gavin argues that it should return true when the result is not zero.
3. I say that the function should be x.sub_and_test_if_zero(v) which returns
true when the result is zero.

In (1), Andrey has provided the function as it is not because this is his
guess of what people want, but because true on zero feels more natural as
it's much more common to test for zero than to test for nonzero after
subtraction. (Not so after a bitwise operation, admittedly.)

In (2), Gavin argues for true on nonzero not because this is what people
want, but because of consistency.

In (3), I suggest the rename not because this is what people want, but
because I want to remove the ambiguity and the need to guess.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
>     if (a.sub_and_test(1))
>
> Also, the generated code for these operations can potentially be more
> efficient.

While we're on the subject, on what architectures would opaque_sub be more
efficient than sub_and_test?


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On January 25, 2018 6:59:52 PM EST, Peter Dimov via Boost <[hidden email]> wrote:

> Frank Mori Hess wrote:
>
> > I haven't been following this specific discussion closely, but in
> general,
> > I wish designers put a higher value on consistency than on trying to
> guess
> > what the audience's expectations are.  Consistency leads to tools
> whose
> > behavior can be reasoned about and competence with can be achieved.
> > Guessing what the people want leads to incomprehensible black boxes
> that
> > the user pokes at, hoping it will guess what they want it to do.
>
> The specific situation under discussion is as follows:
>
> 1. Andrey has provided a function x.sub_and_test(v) which returns true
> when  the result is zero.
> 2. Gavin argues that it should return true when the result is not
> zero.
> 3. I say that the function should be x.sub_and_test_if_zero(v) which
> returns  true when the result is zero.
>
> In (1), Andrey has provided the function as it is not because this is
> his  guess of what people want, but because true on zero feels more
> natural as
> it's much more common to test for zero than to test for nonzero after
> subtraction. (Not so after a bitwise operation, admittedly.)

I don't have the OP handy, but I thought Andrey said it was because that matched the expression he created the function to replace.

> In (2), Gavin argues for true on nonzero not because this is what
> people want, but because of consistency.
>
> In (3), I suggest the rename not because this is what people want, but
> because I want to remove the ambiguity and the need to guess.

I argue that consistency with the other functions is enough to solve this problem since any user of the library must learn its conventions. The rest of the functions are bound to confuse a subset of users, but the greater confusion comes from the function in question testing for a different but value than the rest.

To follow your approach would, I contend, require renaming the other functions in a similar manner. I'm not against that idea, but it is a bigger change.

--
Rob

(Sent from my portable computation device.)

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 26/01/2018 12:03, Peter Dimov wrote:
>> Consistency within this domain should be sufficient.
>
> Consistency and intuition are in conflict here. These function are
> presently specified the way they are specified for a reason - this is
> what makes more sense in the context in which they are typically used.
> You can use consistency to justify a choice, but you can't magically
> make people not be confused by the choice.

I disagree.  Both consistency with other existing methods and intuition
with C++'s conventions (nonzero is truthy) agree that the current
implementation is unexpected.  So there is no conflict there.

Bringing the conventions of other domains (what assembly instructions
do, what the shell 'test' command does, what language X's 'test' keyword
does) into play is a non-argument and not relevant.


"sub_and_test_if_zero" could be a valid (though wordy) name for the
current implementation of the method.

I would prefer if "sub_and_test" (or to prevent breaking changes, an
alternate spelling like "sub_test") would instead return true when
non-zero, thus following both consistency and intuition.

> This is similar to the bool conversion of std::error_code, which is also perfectly consistent and still manages to return the opposite of what a certain fraction of the audience intuitively expects.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
Mere moments ago, quoth I:
> On 26/01/2018 12:03, Peter Dimov wrote:
>> This is similar to the bool conversion of std::error_code, which is
>> also perfectly consistent and still manages to return the opposite of
>> what a certain fraction of the audience intuitively expects.

Sorry, hit Send too soon on the prior reply.

That's a different case; it's taking the "true if non-zero" convention
(the current implementation, based on the fact that internally it's an
integer-like thing) and the "true if non-empty" that is instead
implemented by things like smart pointers and optional (for similar
reasons), and then conflating it with an assumption that "empty ==
success" given that the class name is error_code.  Which is usually but
not always true.  The proposed change is to address that assumption by
treating it less like a number and more like an error state.

But incidentally, error_code's current implementation also goes with the
idea that non-zero is true, so it supports my case. ;)


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 01/26/18 03:08, Peter Dimov via Boost wrote:
>>     if (a.sub_and_test(1))
>>
>> Also, the generated code for these operations can potentially be more
>> efficient.
>
> While we're on the subject, on what architectures would opaque_sub be
> more efficient than sub_and_test?

On x86 and gcc < 7 opaque_sub allows to use "lock sub" or "lock dec"
without setting the bool according to the zero flag, i.e. it saves a
register and an instruction. Gcc 7 introduced the ability to return
flags from the asm statement, so the code can be written the same way.
Although I noticed that the compiler tends to save the flag into a
register early unless it is tested immediately, so in some cases
opaque_sub might still be preferable where it suits. AFAIK, other
compilers, including clang, don't support this feature.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [atomic] (op)_and_test naming

Boost - Dev mailing list
Andrey Semashev wrote:
> > While we're on the subject, on what architectures would opaque_sub be
> > more efficient than sub_and_test?
>
> On x86 and gcc < 7 opaque_sub allows to use "lock sub" or "lock dec"
> without setting the bool according to the zero flag, i.e. it saves a
> register and an instruction.

Right, thanks. I was thinking that testing for zero comes for free, but it's
not (entirely) free for the reason you give. Does this actually matter in
practice? I would expect the atomic to dominate the `set(n)z al`.

> Gcc 7 introduced the ability to return flags from the asm statement, so
> the code can be written the same way. Although I noticed that the compiler
> tends to save the flag into a register early unless it is tested
> immediately, so in some cases opaque_sub might still be preferable where
> it suits.

Don't see how opaque_sub could be preferable if you need to test the flag
later. :-) Presumably, if you just call the function and discard the return
value - the equivalent of opaque_ - the compiler would be smart enough to
not save the flag.

I remember some compilers being smart enough to notice that you don't use
the result of the atomic fetch_op intrinsic and generating the `lock op`
themselves, without a separate opaque_op being needed. We can't do that on
the library level, of course.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
12