Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Yang Li <yang.lee@linux.alibaba.com>, clm@fb.com
Cc:     josef@toxicpanda.com, dsterba@suse.com, nathan@kernel.org,
        ndesaulniers@google.com, linux-btrfs@vger.kernel.org,
        lukas.bulwahn@gmail.com, linux-kernel@vger.kernel.org,
        clang-built-linux@googlegroups.com
References: <1621244810-38832-1-git-send-email-yang.lee@linux.alibaba.com>
From:   Qu Wenruo <quwenruo.btrfs@gmx.com>
Subject: Re: [PATCH v2] btrfs: Remove redundant initialization of 'to_add'
Message-ID: <5938551d-a083-a62c-4ab3-bc29fc62b1df@gmx.com>
Date:   Mon, 17 May 2021 18:13:50 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <1621244810-38832-1-git-send-email-yang.lee@linux.alibaba.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On 2021/5/17 =E4=B8=8B=E5=8D=885:46, Yang Li wrote:
> Variable 'to_add' is being initialized however this value is never
> read as 'to_add' is assigned a new value in if statement. Remove the
> redundant assignment. At the same time, move its declaration into the
> if statement, because the variable is not used elsewhere.
>
> Clean up clang warning:
>
> fs/btrfs/extent-tree.c:2774:8: warning: Value stored to 'to_add' during
> its initialization is never read [clang-analyzer-deadcode.DeadStores]

Personally speaking, compiler should be able to optimize out such
problem, nothing really worthy bothering.

Especially considering these "fixes" just randomly pop up, distracting
the reviewers' time.

If you really believe these "fixes" are really worthy (not to just
fulfill the stupid KPI), please at least pack them into a larger
patchset (but keep the separate patches), not just sending one when you
find one.

>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>

I know some maintainers are already very upset about the bot, although
in your case it reduces a lifespan of a variable, thus it's marginally
acceptable, but under other cases, it doesn't really help much.

If such fixes come from indie developers, I'm pretty fine or even happy
to help them to start more contribution.

But a sponsored bot just repeating clang static analyzer

Trust me, no maintainer will be happy with that, and you're destroying
the reputation of your company (if the reputation hasn't been destoryed
already).

> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
> ---
>
> Change in v2:
> --According to Lukas's suggestion, combine the declaration and assignmen=
t of
>    variable 'to_add' into one line, just as "u64 to_add =3D min(len, ...=
);"
>    https://lore.kernel.org/patchwork/patch/1428697/
>
>   fs/btrfs/extent-tree.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index f1d15b6..13ac978 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2774,11 +2774,9 @@ static int unpin_extent_range(struct btrfs_fs_inf=
o *fs_info,
>   		spin_unlock(&cache->lock);
>   		if (!readonly && return_free_space &&
>   		    global_rsv->space_info =3D=3D space_info) {
> -			u64 to_add =3D len;
> -
>   			spin_lock(&global_rsv->lock);
>   			if (!global_rsv->full) {
> -				to_add =3D min(len, global_rsv->size -
> +				u64 to_add =3D min(len, global_rsv->size -
>   					     global_rsv->reserved);

Have you ever wondered why "global_rsv" is not indented by tab only, but
with extra spaces?

It's supposed to be aligned with "len".

Thanks,
Qu
>   				global_rsv->reserved +=3D to_add;
>   				btrfs_space_info_update_bytes_may_use(fs_info,
>