diff mbox series

[blktests,v1,01/11] nvme/{003,004,005,013,046,049}: Group all variables declarations

Message ID 20230726124644.12619-2-dwagner@suse.de
State New
Headers
Series Switch to allowed_host |

Commit Message

Daniel Wagner July 26, 2023, 12:46 p.m. UTC 
  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(-)

Comments

Bart Van Assche July 26, 2023, 2:54 p.m. UTC | #1 
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.
Daniel Wagner July 27, 2023, 7:11 a.m. UTC | #2 
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.
Bart Van Assche July 27, 2023, 3:18 p.m. UTC | #3 
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.
Daniel Wagner July 28, 2023, 6:46 a.m. UTC | #4 
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.
diff mbox series

Patch

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