linux-kernel - Re: [PATCH blktests v1 01/11] nvme/{003,004,005,013,046,049}: Group all variables declarations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <k3a5yedxk2xxobv7p4ieiyzjbkz3boln3y3hi46hf6agjkd4qp@yqhjcq2moxhe>
Date:   Fri, 28 Jul 2023 07:58:43 +0000
From:   Shinichiro Kawasaki <shinichiro.kawasaki@....com>
To:     Daniel Wagner <dwagner@...e.de>
CC:     Bart Van Assche <bvanassche@....org>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        Chaitanya Kulkarni <kch@...dia.com>,
        Max Gurtovoy <mgurtovoy@...dia.com>,
        Hannes Reinecke <hare@...e.de>,
        Sagi Grimberg <sagi@...mberg.me>,
        James Smart <jsmart2021@...il.com>
Subject: Re: [PATCH blktests v1 01/11] nvme/{003,004,005,013,046,049}: Group
 all variables declarations

On Jul 28, 2023 / 08:46, Daniel Wagner wrote:
> On Fri, Jul 28, 2023 at 05:06:34AM +0000, Shinichiro Kawasaki wrote:
> > On Jul 27, 2023 / 08:18, Bart Van Assche wrote:
> > > On 7/27/23 00:11, Daniel Wagner wrote:
> > > > On Wed, Jul 26, 2023 at 07:54:24AM -0700, Bart Van Assche wrote:
> > > > > On 7/26/23 05:46, Daniel Wagner wrote:
> > > > > > Group all variable declarations together at the beginning of the
> > > > > > function.
> > > > > 
> > > > > An explanation of why this change has been proposed is missing from the
> > > > > patch description.
> > > > 
> > > > Sure, I'll add one. The coding style to declare all local variables at the
> > > > beginning of the function.
> > > 
> > > Isn't declaring local variables just before their first use a better style?
> > 
> > IMO both styles have pros and cons. Declarations at "beginning of functions"
> > helps to understand what the function uses as its local data (pros), but the
> > declaration and the usage are separated and makes it difficult to understand
> > (cons). Declarations at "just before first use" have the opposite pros and cons.
> > This style is easier to read especially when a function is rather long.
> 
> FWIW, if I keep going with the refactoring (providing helper function to
> setup/cleanpup the complete target in one step), most of the tests will be very
> short. Thus there are far less variables to declare anyway.

I can imagine that. Sounds good :)

> 
> > In the past, I preferred declarations at the beginning functions and requested
> > it in my review comments [1], but I learned that this guide is not so widely
> > applied: xfstests scripts, or even blktests 'check' scripts have declarations in
> > the middle of the functions. So I think both styles are okay at this moment.
> 
> Okay, I wasn't aware of this.
> 
> >   [1] https://github.com/osandov/blktests/pull/99
> > 
> > More importantly, this discussion maybe going towards "too strict" guidelines,
> > which will discourage contributions. Similar topic is [[ ]] vs [ ]. Once I was
> > requesting strictly to use [[ ]], but it did not seem productive. Now I no
> > longer request to replace [ ] with [[ ]]. In same manner, I suggest not to be
> > strict on the local variable declaration position either.
> > 
> > As for this patch, it is not required to follow guidelines. Does it make
> > Daniel's refactoring work easier? If so, I guess it will be valuable.
> 
> IMO, this is the case, because you can way easier identify odd balls in the
> large bulk changes where I have to touch almost all tests cases for a change.

I think this reasoning is good enough to have this patch. So, purpose of this
patch is not to follow guidelines but to "find the odd balls" and make
refactoring easier.

> 
> So ideally, after these refactoring most of the tests will be shorter. Thinking
> about this, I could first introduce these helpers and update the callsides.
> Though I find this harder to review because all the tests look slightly
> different. But hey there are more one road to reach Rome. I suspect this
> approach would reduce the code churn a bit. Anyway, let me know what you prefer.

The road you chose looks the fastest way for me.