Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

[Vladimir Sementsov-Ogievskiy]

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,
Vladimir