lkml.org 
[lkml]   [2023]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH blktests v1 01/11] nvme/{003,004,005,013,046,049}: Group all variables declarations
Date
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.
\
 
 \ /
  Last update: 2023-07-28 10:01    [W:0.039 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site