Re: [Tarantool-patches] [PATCH 5/6] txn: stop using txn_has_flag

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Sat, Jan 30, 2021 at 08:17:59PM +0100, Vladislav Shpilevoy wrote:
> > --- a/src/box/txn.c
> > +++ b/src/box/txn.c
> > @@ -526,7 +526,7 @@ txn_free_or_wakeup(struct txn *txn)
> >  void
> >  txn_complete_fail(struct txn *txn)
> >  {
> > -	assert(!txn_has_flag(txn, TXN_IS_DONE));
> > +	assert(!(txn->flags & TXN_IS_DONE));
> 
> Please, use explicit != 0. We don't apply '!' operator to
> non-boolean values. The same in other places. This I can even
> find in the code style guide:
> 
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style

I remember this. And used this style initially. But with this rule applied
code becomes a way more ugly. For example

-       if (!txn_has_flag(txn, TXN_CAN_YIELD))
+       if ((txn->flags & TXN_CAN_YIELD) == 0)

In first place a person notes the "logical not" operator immediately,
and this sounds more natural than excessive five symbols at the tail of
the 'if' statement.

Another example

-       assert(!txn_has_flag(txn, TXN_IS_DONE));
-       assert(!txn_has_flag(txn, TXN_WAIT_SYNC));
+       assert(!(txn->flags & (TXN_IS_DONE | TXN_WAIT_SYNC)));

Which should be changed to either

	assert((txn->flags & (TXN_IS_DONE | TXN_WAIT_SYNC)) == 0);

or back to pair

	assert((txn->flags & TXN_IS_DONE) == 0);
	assert((txn->flags & TXN_WAIT_SYNC) == 0);

which is a way more worse than it was with txn_has_flag() helper,
at least for me.

The initial rationale for this series was (as far as I remember) to
setup several flags at once, so I think you could consider implementing
txn_set_flags() helper which would do the trick instead. Thus lets drop
this series, it doesn't make anything better without using neg operator.