| Message ID | 20230726124644.12619-2-dwagner@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Switch to allowed_host | expand |
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. I think the current style, with variable declarations occurring just before the first use of a variable, is on purpose. I like that style. Bart.
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. > I think the current style, with variable declarations occurring just before > the first use of a variable, is on purpose. I like that style. The majority of functions declare variables at the beginning of the functions. As you can see these are only a handful of declerations which do not adhere to the coding style.
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? Thanks, Bart.
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. 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. [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.
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.
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.
diff --git a/tests/nvme/003 b/tests/nvme/003 index 6604012d2068..aa26abf8d8b3 100755 --- a/tests/nvme/003 +++ b/tests/nvme/003 @@ -22,10 +22,11 @@ test() { _setup_nvmet + local loop_dev local port + port="$(_create_nvmet_port "${nvme_trtype}")" - local loop_dev loop_dev="$(losetup -f)" _create_nvmet_subsystem "blktests-subsystem-1" "${loop_dev}" diff --git a/tests/nvme/004 b/tests/nvme/004 index cab98ff44326..1e5c2b8b3e87 100755 --- a/tests/nvme/004 +++ b/tests/nvme/004 @@ -23,11 +23,12 @@ test() { _setup_nvmet local port + local loop_dev + port="$(_create_nvmet_port "${nvme_trtype}")" truncate -s "${nvme_img_size}" "$TMPDIR/img" - local loop_dev loop_dev="$(losetup -f --show "$TMPDIR/img")" _create_nvmet_subsystem "blktests-subsystem-1" "${loop_dev}" \ diff --git a/tests/nvme/005 b/tests/nvme/005 index 8e15a13f3794..836854086822 100755 --- a/tests/nvme/005 +++ b/tests/nvme/005 @@ -22,11 +22,13 @@ test() { _setup_nvmet local port + local loop_dev + local nvmedev + port="$(_create_nvmet_port "${nvme_trtype}")" truncate -s "${nvme_img_size}" "$TMPDIR/img" - local loop_dev loop_dev="$(losetup -f --show "$TMPDIR/img")" _create_nvmet_subsystem "blktests-subsystem-1" "${loop_dev}" \ @@ -35,7 +37,6 @@ test() { _nvme_connect_subsys "${nvme_trtype}" blktests-subsystem-1 - local nvmedev nvmedev=$(_find_nvme_dev "blktests-subsystem-1") udevadm settle diff --git a/tests/nvme/013 b/tests/nvme/013 index 14e646a19c47..2be8681616d1 100755 --- a/tests/nvme/013 +++ b/tests/nvme/013 @@ -26,7 +26,6 @@ test() { local port local nvmedev local file_path="${TMPDIR}/img" - local subsys_name="blktests-subsystem-1" truncate -s "${nvme_img_size}" "${file_path}" diff --git a/tests/nvme/046 b/tests/nvme/046 index b37b9e98a559..942f25206c17 100755 --- a/tests/nvme/046 +++ b/tests/nvme/046 @@ -16,6 +16,7 @@ requires() { test_device() { echo "Running ${TEST_NAME}" + local ngdev=${TEST_DEV/nvme/ng} local perm nsid diff --git a/tests/nvme/049 b/tests/nvme/049 index f72862c6426d..599ab58d7a29 100755 --- a/tests/nvme/049 +++ b/tests/nvme/049 @@ -17,6 +17,7 @@ requires() { test_device() { echo "Running ${TEST_NAME}" + local ngdev=${TEST_DEV/nvme/ng} local common_args=( --size=1M
Group all variable declarations together at the beginning of the function. Signed-off-by: Daniel Wagner <dwagner@suse.de> --- tests/nvme/003 | 3 ++- tests/nvme/004 | 3 ++- tests/nvme/005 | 5 +++-- tests/nvme/013 | 1 - tests/nvme/046 | 1 + tests/nvme/049 | 1 + 6 files changed, 9 insertions(+), 5 deletions(-)