From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7D4A3445320 for ; Sat, 11 Jul 2020 17:10:36 +0300 (MSK) References: <20200710075605.217824-1-gorcunov@gmail.com> <20200710075605.217824-4-gorcunov@gmail.com> <9c941738-75ce-3357-1bdb-6af6b95a4db6@tarantool.org> <20200710211020.GI1999@grain> <66e76c8e-3c53-9336-9c37-48dee8fefee8@tarantool.org> <20200710213601.GA296695@grain> From: Vladislav Shpilevoy Message-ID: <4452d48a-ff40-4fbe-b694-7a6579755299@tarantool.org> Date: Sat, 11 Jul 2020 16:10:34 +0200 MIME-Version: 1.0 In-Reply-To: <20200710213601.GA296695@grain> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml 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 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 variables in the beginning like in C89. Isn't it bad taste? - I 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 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.