From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f194.google.com (mail-lj1-f194.google.com [209.85.208.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1C2BC445320 for ; Sat, 11 Jul 2020 18:18:45 +0300 (MSK) Received: by mail-lj1-f194.google.com with SMTP id s9so9747589ljm.11 for ; Sat, 11 Jul 2020 08:18:45 -0700 (PDT) Date: Sat, 11 Jul 2020 18:18:41 +0300 From: Cyrill Gorcunov Message-ID: <20200711151841.GB296695@grain> 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> <4452d48a-ff40-4fbe-b694-7a6579755299@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4452d48a-ff40-4fbe-b694-7a6579755299@tarantool.org> 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: Vladislav Shpilevoy Cc: tml 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.