On Fri, Jan 08, 2021 at 01:17:57PM -0500, Taylor Blau wrote: > 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. > 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
