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

[Shinichiro Kawasaki]

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.