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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On 31.01.2021 23:13, Cyrill Gorcunov wrote:
> 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.

Another purpose of the issue was to make TXN_WAIT_SYNC a part of
TXN_WAIT_ACK. Because ACK is never present without SYNC.

Talking of the setting many flags at once - you still can do this,
even with txn_set_flag(). Just rename txn_set_flag() to txn_set_flags(),
and make the flags proper bitfields like in the first commits of this
series. Then we could do

	txn_set_flags(TXN_WAIT_SYNC | TXN_WAIT_ACK)

Although I don't know what to do with checking flags. If we add
txn_has_flags(), then should it return true if all the requested
flags are present, or if any of them?