Re: [Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Sat, Jul 11, 2020 at 04:10:34PM +0200, Vladislav Shpilevoy wrote:
> On 10/07/2020 23:36, Cyrill Gorcunov wrote:
> > On Fri, Jul 10, 2020 at 11:28:26PM +0200, Vladislav Shpilevoy wrote:
> >> On 10/07/2020 23:10, Cyrill Gorcunov wrote:
> >>> On Fri, Jul 10, 2020 at 10:35:50PM +0200, Vladislav Shpilevoy wrote:
> >>>> Опять же. Патч технически корректен, но так же и бесполезен.
> >>>> Его полезность сравнима с исправлением опечатки в каком-нибудь
> >>>> не особо нужном комменте. Сори, если токсично звучит.
> >>>>
> >>>> Но не вижу в этом патче нужды.
> >>>
> >>> We've to allocate a variable which we simply don't need. Look,
> >>> the former code reads the flag, puts it into variable then
> >>> immediately read it and that's all :/ I think we should not
> >>> spread @is_sync but read the flag as much as possible for
> >>> better grep'ability.
> >>
> >> Do you have a proof that this change improves anything? That
> >> the variable 'allocation' on the stack actually happens, and
> >> costs even 1 ns?
> > 
> > You simply don't need it. I think the compiler will rip it off.
> > Again, if you suspect that the code gonna be changed then ignoring
> > the commit is perfectly fine. If not -- the useless variable is just
> > a bad taste at minimum.
> 
> So we came to a conclusion that the patch has nothing to do with
> perf. It is just subjective feeling of taste. Sorry, but I am not

This _have_ impact on debug builds at least. Look, the problem is
that here is what we've done

	a = b
	if (a)
		...

iow we've allocated a variable, use it immediately and that's all. We
don't even have one more usage. Why do we acllocate it at all?
Why we need this variable? Why can't we read @b directly but use
a temporary variable instead? Maybe the @b is being modifying externally
so we need a local immutable copy?

> buying it. If everyone in the team would rewrite every place in
> the source code they consider 'bad taste', we would end up
> rewriting each other's code 100% of work time.
> 
> Btw, talking of bad taste of mine - you yourself declare all

I didn't said that you have a bad taste, I pointed that if we leave
it in this form forever it gonna be a bad taste simply because every
line in source code must have a meaning, but with this redundant
variable there is no any meaning simply because "it just happen this way".
This is perfectly fine for initial commits where we're consider various
design for features and rolling code to and from.

I don't get why you take it so personal. I didn't mean to offence.

> variables in the beginning like in C89. Isn't it bad taste? - I

This has a reason -- not always compiler warns about shadow variables.
But if you look into my commits you'll see that this is not mandatory
for me either, I use both.

> don't know. Or is it a bad taste to do the same one-line change
> in two neighbor functions in 2 separate commits like removal of
> 
>     (void) txn_limbo;
> 
> ? This easily could be one commit, easier to review, and less commits
> to skip in git log, when look for something. I don't know whether it

And this has a reason as well -- I wanted to make changes small because
you've been modifying source code and when you need to apply patches
you could easily resolve conflicts. Moreover you could merge the small
patches then.

> is a bad taste, because I don't have definition of bad taste. And you
> either.
> 
> Even if I feel I would do something a bit different, it is certainly
> not a reason for me now to go and "fix" all such places. Just because
> I consider them not beautiful enough. There is a sanity border after
> which refactoring becomes useless or even harmful when it comes to git
> history and waste of time on that.