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.
