Re: [PATCH v5 4/6] pack-bitmap: prepare to read lookup table extension

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jul 25, 2022 at 9:21 PM Taylor Blau <me@ttaylorr.com> wrote:
> On Wed, Jul 20, 2022 at 06:38:22PM +0000, Abhradeep Chakraborty via GitGitGadget wrote:
> > +     triplet->commit_pos = get_be32(p);
> > +     p += sizeof(uint32_t);
> > +     triplet->offset = get_be64(p);
> > +     p += sizeof(uint64_t);
> > +     triplet->xor_row = get_be32(p);
> > +     return 0;
>
> Just noticing this now, but I wonder if we could avoid incrementing `p`
> here and instead write something like:
>
>     triplet->commit_pos = get_be32(p);
>     triplet->offset = get_be64(p + sizeof(uint32_t));
>     triplet->xor_row = get_be64(p + sizeof(uint64_t) + sizeof(uint32_t));
>
> I don't have a strong feeling about it, though, it just seems to read a
> little more directly to me and avoid modifying a variable that is only
> going to live as long as the function executes (p).

While it may not matter much in this tiny function, the code in the
patch sets a better precedent by conforming to a pattern which is more
maintainable in situations involving more pieces of data which need to
be decoded. It's also easier to reason about than the suggested
replacement since you don't have to spend extra cycles double-checking
if it's adding the correct number of and correctly-sized offsets at
each get_be*() invocation. IMHO, the way the patch already handles
this seems preferable.