Re: [PATCH v2 6/6] pack-bitmap[-write]: use `object_array_clear()`, don't leak
- From
Jeff King <peff@peff.net>
- Date
- Sep 23, 2017, 04:35 UTC
- Message-ID
- <20170923043532.in6pcrf7roxhfrrq@sigill.intra.peff.net>
- In-Reply-To
- <30407fada0ee65ca61e5cc52b1dafc708ab90883.1506120292.git.martin.agren@gmail.com>
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...).
Show 5 quoted lines
> 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.
Show 5 quoted lines
> 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