Re: [PATCH] http.c: clear the 'finished' member once we are done with it

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 25, 2022, 13:27 UTC
Message-ID: <220525.86bkvlu4bx.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqh75eef0f.fsf@gitster.g>

On Tue, May 24 2022, Junio C Hamano wrote:

Show 16 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> It doesn't mean that GCC has additionally proved that we'll later used
>> it in a way that will have a meaningful impact on the behavior of our
>> program, or even that it's tried to do that. See an excerpt from the GCC
>> code (a comment) in [1].
>
> But that means the warning just as irrelevant as "you stored 438 to
> this integer variable".  Sure, there may be cases where that integer
> variable should not exceed 400 and if the compiler can tell us that,
> that would be a valuable help to developers.  But "you stored an
> address of an object that can go out of scope in another object
> whose lifetime lasts beyond the scope" alone is not, without "and
> the caller that passed the latter object later dereferences that
> address here".  We certainly shouldn't -Werror on such a warning
> and bend our code because of it.

I think it says something that 1) we had exactly one of these in our codebase 2) as we've discussed the pointer isn't actually *needed* outside the scope of the function, it's just left-over.

Now, if it were used, e.g. let's say we had some code that took the struct and inspected its members we'd likely have a segfault here, or worse it would "work", but only on the platforms we'd test at first.

Which isn't the case with a leftover "int finished" holding a 438.

The point of this warning, like so many others, is to ask "hey, do you really need to be running around with this particular pair of scissors?".