Re: [PATCH v2 6/6] pack-bitmap[-write]: use `object_array_clear()`, don't leak

[Jeff King <peff@peff.net>]

On Sat, Sep 23, 2017 at 01:34:54AM +0200, Martin Ågren wrote:

> Instead of setting the fields of rev->pending to 0/NULL, thereby leaking
> memory, call `object_array_clear(&rev->pending)`.

I wondered if these cases might sometimes work on an uninitialized
struct (so setting the fields unconditionally without looking at them
would be the right thing). But nope, they are always initialized by the
revision machinery. So freeing is the right thing.

Like the earlier patch, we'd want to be sure there are no dangling
pointers here to the name/path fields. But I looked over the code and I
feel confident there are not (famous last words...).

> In pack-bitmap.c, we make copies of those fields as `pending_nr` and
> `pending_e`. We never update the aliases and the original fields never
> change, so the aliases are not really needed and just make it harder
> than necessary to understand the code. While we're here, remove the
> aliases to make the code easier to follow.

Agreed. Sometimes aliasing like this helps readability when the original
variable is wordy to access. But I don't think that is the case here.

It _can_ also allow the compiler to optimize harder (e.g., by putting
"pending_nr" into a register, because it's not 100% sure that
revs->pending.nr is unchanged by function calls within the loop). But
unless we know something is a tight loop, I don't think that level of
micro-optimization is worth the readability hit.

> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  pack-bitmap-write.c |  4 +---
>  pack-bitmap.c       | 10 +++-------
>  2 files changed, 4 insertions(+), 10 deletions(-)

Patch itself is as advertised. Looks good.

-Peff