Re: [Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Sat, Nov 21, 2020 at 12:55:16AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> NACK.

Look, there must be a serious reason why you use temp variable instead
of direct access to the variable itself. They could be

1) The variable might be changed externally after you fetched it
   into another place (which means you might to need a compiler
   barrier as well but it depends, sometime even compiler help
   is not enough and you need hw barrier).

2) The variable is immutable but its name is too long and you use
   temp variable as an alias (which compiler most likely to optimize).

   If code usage in this case is too big then this become unacceptable
   and better redesign the code.

3) It simply happened this way.

The reason 1) and 2) are pretty usable all over the world. In this
particular code we've (3). So I'm asking you "why do you need a temp
variable here?"

When we've been reviewing the whole qsync series I already told
that we better should use direct access to the flags all the
time until there is a reson 1) or 2) to cache the value.

But we've been under the time pressure then and I simply closed
my eyes. Since I'm working on this code right now I hate with
a passion such aggressive caching for NO reason, this even
makes flags grepping more hard - instead of validating flags
access you need to walk two ways: find flag read operation
then find the use of a new variable... and I wonder do you
*really* thing this is normal practice?