Re: [RFC PATCH 5/6] object-name: use hexval
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Aug 25, 2026, 21:41 UTC
- Message-ID
- <ao4MJQgp6Ai4tJxi@fruit.crustytoothpaste.net>
- In-Reply-To
- <CABPp-BFDaWdahoOnNRGQjshzQXin1YLuROv94W_PrajnLWDAuQ@mail.gmail.com>
On 2026-08-25 at 19:44:34, Elijah Newren wrote:
Show 12 quoted lines
> On Wed, Jul 29, 2026 at 4:33 PM brian m. carlson > <sandals@crustytoothpaste.net> wrote: > > > > We've open-coded a different implementation of parsing hex values here > > when we already have a perfectly good one in hexval. This > > implementation will almost certainly be slower because it isn't > > table-driven, unlike the other one, and since it's not constant time it > > has no other advantages either. To tidy things up and prepare for > > future work, switch to hexval in this case. > > As Junio noted, you may want to call out that your replacement drops > the case-normalization that the former parse_oid_prefix() provided.
Will fix in v2.
Show 19 quoted lines
> [...]
> > - unsigned char val;
> [...]
> > + int val = hexval(c, HEX_KIND_OID);
> > +
> > + if (val < 0)
> > return -1;
> [...]
> > if (oid_out) {
> > if (!(i & 1))
> > val <<= 4;
> > oid_out->hash[i >> 1] |= val;
>
> hexval returns unsigned int. Is there a risk that someone "tries to
> fix" that discrepancy by changing val to unsigned int here,
> inadvertently causing the `if` immediately below to become dead code?
>
> In patch 1, in hex2chr, you used a (val & ~0xf) check together with an
> unsigned int val; would that make sense here, or is that overkill?I can re-roll with an appropriate change, sure. I think that we'd need to have a slightly different check, but I'll tidy it up accordingly.
-- brian m. carlson (they/them) Toronto, Ontario, CA