On 2024-11-07 at 01:39:15, Jeff King wrote:
> So I think you wouldn't want to allocate an enum or a slot in the
> hash_algos array, because it is not really an independent algorithm.
> But I think it _could_ work as a separate struct that the caller derives
> from the main hash algorithm. For example, imagine that the
> git_hash_algo struct didn't have unsafe_init_fn, etc, but instead had:
>
> struct git_hash_algo *unsafe_implementation;
>
> with a matching accessor like:
>
> struct git_hash_algo *unsafe_hash_algo(struct git_hash_algo *algo)
> {
> /* if we have a faster "unsafe" implementation, use that */
> if (algo->unsafe_implementation)
> return algo->unsafe_implementation;
> /* otherwise just use the default one */
> return algo;
> }
>
> And then csum-file.c, rather than calling:
>
> the_hash_algo->unsafe_init_fn(...);
> ...
> the_hash_algo->unsafe_final_fn(...);
>
> would do this:
>
> struct git_hash_algo *algo = unsafe_hash_algo(the_hash_algo);
> algo->init_fn(...);
> ...
> algo->final_fn(...);
>
> And likewise this test helper would have a single conditional at the
> start:
>
> if (unsafe)
> algo = unsafe_hash_algo(algo);
>
> and the rest of the code would just use that.
Ah, yes, I think that approach would be simpler and nicer to work with
than a separate entry in the `hash_algos` array. We do, however, have
some places that assume that a `struct git_hash_algo *` points into the
`hash_algos` array (notably `hash_algo_by_ptr`), so we'd have to adjust
for that, move the function pointers out into their own struct which
we'd use for `unsafe_hash_algo`, or be careful never to call the
relevant functions on our special `git_hash_algo` struct.
> All that said, I do not think it buys us anything for "real" code. There
> the decision between safe/unsafe is in the context of how we are using
> it, and not based on some conditional. So while the code in this test
> helper is ugly, I don't think we'd ever write anything like that for
> real. So it may not be worth the effort to refactor into a more virtual
> object-oriented way.
Yeah, I don't have a strong opinion one way or the other. I think, with
the limitation I mentioned above, it would probably require a decent
amount of refactoring if we took a different approach, and I'm fine with
going with Taylor's current approach unless he wants to do that
refactoring (in which case, great).
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
Attachment:
signature.asc
Description: PGP signature
