Group all variable declarations together at the beginning of the
function.
Signed-off-by: Daniel Wagner <[email protected]>
---
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(-)
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
--
2.41.0
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.