On Mon, Jul 25, 2022 at 9:21 PM Taylor Blau <me@xxxxxxxxxxxx> 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.
