On Fri, Jan 10, 2025 at 04:38:56PM -0500, Taylor Blau wrote:
> On Thu, Nov 21, 2024 at 04:37:31AM -0500, Jeff King wrote:
> > If we don't care about the speed of this function, then an
> > implementation like:
> >
> > for (i = 0; i < GIT_HASH_NALGOS; i++) {
> > if (p == &hash_algos[i] || p == hash_algos[i]->unsafe)
> > return i;
> > }
> > return GIT_HASH_UNKNOWN;
> >
> > would work. I'm not sure if that would be measurable. I was surprised at
> > the number of places that hash_algo_by_ptr() is called. Many low-level
> > oid functions need it because we store the integer id there rather than
> > a direct pointer (so oidread(), oidclr(), oid_object_info_extended(),
> > and so on). But I'd also expect the loop above to be pretty fast. So I
> > dunno.
>
> Concerns about the speed aside (I agree that the for loop is likely to
> be very fast, and will probably get unrolled by modern compilers), this
> looks good to me with one small tweak.
It should definitely be unrolled. The big change against the existing
code is that it has branches. But again, I'm not sure how much the
performance matters here (I would have naively said not at all, but it
does get called in lots of low-level spots).
> We can't use `hash_algos[i]->unsafe` directly it might be NULL, so the
> function as above would change the behavior of hash_algo_by_ptr()
> slightly when provided NULL (it would return SHA-256 instead of
> UNKNOWN).
>
> So I think you'd want to write the loop like:
>
> size_t i;
>
> for (i = 0; i < GIT_HASH_NALGOS) {
> const struct git_hash_algo *algop = &hash_algos[i];
>
> if (p == algop || (algop->unsafe && p == algop->unsafe))
> return i;
> }
>
> return GIT_HASH_UNKNOWN;
Yes, that works. Or maybe even just:
if (!algop)
return GIT_HASH_UNKNOWN;
at the top to cover the special case.
-Peff
