[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. T

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.
Date: Thu, 30 Sep 2021 15:18:38 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.0

9/30/21 12:46, Kevin Wolf wrote:
Am 29.09.2021 um 07:30 hat ~farzon geschrieben:
From: Farzon Lotfi <hi@farzon.org>

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371

Signed-off-by: Farzon Lotfi <hi@farzon.org>

Just picking one example, but it applies to most hunks of the patch:

diff --git a/block/parallels.c b/block/parallels.c
index 6ebad2a2bb..629d8aae2b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
static BlockDriver bdrv_parallels = {
-    .format_name       = "parallels",
-    .instance_size     = sizeof(BDRVParallelsState),
-    .bdrv_probe                = parallels_probe,
-    .bdrv_open         = parallels_open,
-    .bdrv_close                = parallels_close,
+    .format_name    = "parallels",
+    .instance_size  = sizeof(BDRVParallelsState),
+    .bdrv_probe     = parallels_probe,
+    .bdrv_open      = parallels_open,
+    .bdrv_close     = parallels_close,
      .bdrv_child_perm          = bdrv_default_perms,
      .bdrv_co_block_status     = parallels_co_block_status,
      .bdrv_has_zero_init       = bdrv_has_zero_init_1,

When we're changing these lines anyway, let's make sure to have
consistent alignment with the surrounding code. So I would prefer
something like:

+    .bdrv_close               = parallels_close,
      .bdrv_child_perm          = bdrv_default_perms,

Rather than:

+    .bdrv_close     = parallels_close,
      .bdrv_child_perm          = bdrv_default_perms,

In most cases, there are already inconsistencies in the BlockDriver
definitions, but let's use the chance to make it a little better.

Or may be drop alignment around '=' at all, to have

   .bdrv_child_perm = bdrv_default_perms,
   .bdrv_co_block_status = parallels_co_block_status,
   .bdrv_has_zero_init = bdrv_has_zero_init_1,


This alignment in definitions is

1. source for alignment inconsistencies
2. infecting logic-changing patches by fixing surround alignment (or having to 
add separate patches to adjust old alignments, which is a real waste of time)
3. [1] and [2] will never be helpful for rebase conflicts resolution, when need 
to backport/forwardport commits.

and for that all we have a bit more readable definition, which is rarely read as is. 
(I think, it's more often used to navigate by tags, like bdrv_open -> jump to 
invocation in qcow2.c -> jump to qcow2_open)

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]