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

quoted

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

quoted

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