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

[Daniel Wagner]

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.

> 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.

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.