[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 6/7] block: Clean up local variable shadowing
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 6/7] block: Clean up local variable shadowing |
Date: |
Mon, 18 Sep 2023 16:49:58 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
Kevin Wolf <kwolf@redhat.com> writes:
> Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
>> Local variables shadowing other local variables or parameters make the
>> code needlessly hard to understand. Tracked down with -Wshadow=local.
>> Clean up: delete inner declarations when they are actually redundant,
>> else rename variables.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> block.c | 7 ++++---
>> block/rbd.c | 2 +-
>> block/stream.c | 1 -
>> block/vvfat.c | 34 +++++++++++++++++-----------------
>> hw/block/xen-block.c | 6 +++---
>> 5 files changed, 25 insertions(+), 25 deletions(-)
>
> I wonder why you made vdi a separate patch, but not vvfat, even though
> that has more changes. (Of course, my selfish motivation for asking this
> is that I could have given a R-b for it and wouldn't have to look at it
> again in a v2 :-))
I split by maintainer. The files changed by this patch are only covered
by "Block layer core".
>> diff --git a/block.c b/block.c
>> index a307c151a8..7f0003d8ac 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3001,7 +3001,8 @@ static BdrvChild
>> *bdrv_attach_child_common(BlockDriverState *child_bs,
>> BdrvChildRole child_role,
>> uint64_t perm, uint64_t
>> shared_perm,
>> void *opaque,
>> - Transaction *tran, Error **errp)
>> + Transaction *transaction,
>> + Error **errp)
>> {
>> BdrvChild *new_child;
>> AioContext *parent_ctx, *new_child_ctx;
>> @@ -3088,7 +3089,7 @@ static BdrvChild
>> *bdrv_attach_child_common(BlockDriverState *child_bs,
>> .old_parent_ctx = parent_ctx,
>> .old_child_ctx = child_ctx,
>> };
>> - tran_add(tran, &bdrv_attach_child_common_drv, s);
>> + tran_add(transaction, &bdrv_attach_child_common_drv, s);
>>
>> if (new_child_ctx != child_ctx) {
>> aio_context_release(new_child_ctx);
>
> I think I would resolve this one the other way around. 'tran' is the
> typical name for the parameter and it is the transaction that this
> function should add things to.
>
> The other one that shadows it is a local transaction that is completed
> within the function. I think it's better if that one has a different
> name.
>
> As usual, being more specific than just 'tran' vs. 'transaction' would
> be nice. Maybe 'aio_ctx_tran' for the nested one?
Can do.
> The rest looks okay.
Thanks!