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.
