Re: [PATCH 18/20] pack-revindex: remove unused 'find_revindex_position()'

From: Jeff King <peff@peff.net>
Date: Jan 12, 2021, 09:32 UTC
Message-ID: <X/1st6SrJXysoejt@coredump.intra.peff.net>
In-Reply-To: <d60411d524656f4680ac578765b2a8704325a060.1610129796.git.me@ttaylorr.com>

On Fri, Jan 08, 2021 at 01:17:57PM -0500, Taylor Blau wrote:

Show 8 quoted lines

> Now that all 'find_revindex_position()' callers have been removed (and
> converted to the more descriptive 'offset_to_pack_pos()'), it is almost
> safe to get rid of 'find_revindex_position()' entirely. Almost, except
> for the fact that 'offset_to_pack_pos()' calls
> 'find_revindex_position()'.
> 
> Inline 'find_revindex_position()' into 'offset_to_pack_pos()', and
> then remove 'find_revindex_position()' entirely.

Sounds good.

Show 7 quoted lines

> This is a straightforward refactoring with one minor snag.
> 'offset_to_pack_pos()' used to load the index before calling
> 'find_revindex_position()'. That means that by the time
> 'find_revindex_position()' starts executing, 'p->num_objects' can be
> safely read. After inlining, be careful to not read 'p->num_objects'
> until _after_ 'load_pack_revindex()' (which loads the index as a
> side-effect) has been called.

Good catch. We might want to drop the initialization of "lo":

>  	int lo = 0;
> -	int hi = p->num_objects + 1;

down to here:

> +	hi = p->num_objects + 1;

to maintain symmetry (though it's quite a minor point).

I notice these are signed ints, but we've taken care to use uint32_t elsewhere for positions. Shouldn't these be uint32_t, also (or at least unsigned)?

-Peff