Re: Proposal/Discussion: Turning parts of Git into libraries

From: Jeff King <peff@peff.net>
Date: Feb 22, 2023, 19:25 UTC
Message-ID: <Y/ZsFuTKyfR+AQy5@coredump.intra.peff.net>
In-Reply-To: <CAJoAoZm+TkCL0Jpg_qFgKottxbtiG2QOiY0qGrz3-uQy+=waPg@mail.gmail.com>

On Tue, Feb 21, 2023 at 02:06:55PM -0800, Emily Shaffer wrote:

> > Does removing memory leaks also mean converting UNLEAK to free()?
> 
> I suspect so - as I understand it, UNLEAK is a macro that resolves to
> "don't complain to me, compiler, I meant not to free it."

Correct. It is supposed to be used sparingly at the outermost level to say "I'm about to exit, so yes, we are leaking this, but no, it does not matter".

So...

Show 10 quoted lines

> > Thinking of things in a library context probably pushes us in that
> > direction (though, alternatively, it might just highlight the question
> > of what is considered "low-level" instead).
> 
> I'm not sure whether use of UNLEAK has so much to do with "low-level"
> or not. In cases when Git is being called as an ephemeral single-run
> process, UNLEAK makes a lot of sense. In cases when Git is being
> called in a long-lived process, UNLEAK is just a sign that says
> "there's a leak here".  So I think the distinction is not low-level or
> high-level, but more simply, within a library or not.

I'd take "low-level" here to mean "far down in the call stack". That is, code which is called potentially from a lot of places, and can't know what is going to happen afterwards.

In that case, such code calling UNLEAK() is already doing the wrong thing. And such code is a likely candidate for being called in a lib-ified long-running process, which means that ignoring the leaks is likely to be more noticeable. :)

There are probably cases where code that is currently high-level becomes more low-level, and will need to be adapted. For example, if cmd_diff() has a static-local helper function for "diff these two blobs", and it knows it will run it exactly once, it is OK to UNLEAK() from there now. But that may be a reasonable API to expose more widely, at which point it needs to stop UNLEAK()-ing and really free.

Just my two cents as the originator of UNLEAK(). :)

-Peff