On Mon, Nov 11, 2024 at 11:38:50AM +0100, Patrick Steinhardt wrote: > There are two users of `UNLEAK()` left in our codebase: > > - In "builtin/clone.c", annotating the `repo` variable. That leak has > already been fixed though as you can see in the context, where we do > know to free `repo_to_free`. > > - In "builtin/diff.c", to unleak entries of the `blob[]` array. That > leak has also been fixed, because the entries we assign to that > array come from `rev.pending.objects`, and we do eventually release > `rev`. Yay, as the author of UNLEAK() I'm in favor of getting rid of it. > This neatly demonstrates one of the issues with `UNLEAK()`: it is quite > easy for the annotation to become stale. A second issue is that its > whole intent is to paper over leaks. And while that has been a necessary > evil in the past, because Git was leaking left and right, it isn't > really much of an issue nowadays where our test suite has no known leaks > anymore. I do agree that stale annotations are a weakness (they do not hurt the leak-checker if they exist, but they are an eye-sore). I'm not sure I would agree that the intent was to paper over leaks. The point was to avoid noise from the leak-checker about memory that was intentionally held until program exit and released by returning from main(). I think the main thing that made it obsolete is that we decided it was worth it to spend the cycles freeing that memory rather than ignoring it. But it's possible I'm just splitting hairs. :) At any rate, for whatever reason people did not seem to use it as intended, and _did_ end up annotating real leaks (papering over them). I guess I could have done a better job of explaining it. -Peff
