[Tarantool-patches] [PATCH 5/6] txn: stop using txn_has_flag
Cyrill Gorcunov
gorcunov at gmail.com
Mon Feb 1 01:13:17 MSK 2021
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.
More information about the Tarantool-patches
mailing list