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

From: Michael J Gruber <git@grubix.eu>
Date: May 25, 2022, 09:08 UTC
Message-ID: <165346968167.4313.13200529030347354219.git@grubix.eu>
In-Reply-To: <xmqqh75eef0f.fsf@gitster.g>

Junio C Hamano venit, vidit, dixit 2022-05-25 00:34:40:

Show 11 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.

An integer can hold 438 perfectly, without any help by carefully designed code.

Show 6 quoted lines

> 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.

A global variable cannot hold a meaningful pointer to a local variable, unless the carefully designed code helps. So that "analogy" rather highlights the essential difference, unless you think about a pointer as "just some number" rather than "something that can be dereferenced".

[read global as outer scope, local as inner scope for simplicity]

Common practice is not necessarily good practice. In a traditional C mindset, everything around pointers and memory management is doomed to boom unless the code is designed carefully and "you know what you are doing". I'm not indicating that you do not - on the contrary, you very well do, as your detailed analysis of the code flow shows. At the same time, it shows that we cannot be certain about that piece of code without that detailed expert analysis.

C is not C++ nor rust, nor should it be; but the warnings and errors in newer standards typically try to avoid those pitfalls by making sure that, e.g., pointers do not go stale for "obvious reasons". They might missflag cases where this is preempted for non-obvious reasons, but forcing you to be explicit (more obvious) in your code is a good thing, especially for maintainability of the code base.

Pointing from outer scope to memory in an inner scope should be a no-go; that's what this error is about. Unsetting that pointer (by setting the pointer to NULL) right before the inner scope ends is exactly the right solution. If this "breaks" the code, the code is broken already.

Ironically, my original one line patch seems to work here, as your detailed analysis shows. Truth in advertising: I arrived at that patch after a considerably less detailed analysis ;)

Michael