On Thu, Nov 07, 2024 at 01:49:35AM +0000, brian m. carlson wrote: > 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. Yeah, I wondered if some code might be surprised by having a separate hash algo. Another weird thing is that the sub-implementation algo struct will have its own rawsz, hexsz, etc, even though those _must_ be the same its parent. If all of the virtual implementation functions were in a git_hash_impl struct, then each git_hash_algo struct could have one embedded for the main implementation (which in sha1's case would be a collision detecting one), and an optional pointer to another unsafe _impl struct. And then you get more type-safety, because you can never confuse the _impl struct for a parent git_hash_algo. The downside is that every single caller, even if it doesn't care about the unsafe variant, needs to refer to the_hash_algo->impl.init_fn(), etc, due to the extra layer of indirection. Probably not worth it. > 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). Me too. If we were fixing something ugly or error-prone that we expected to come up in real code, it might be worth it. But it's probably not for trying to accommodate a one-off test helper. -Peff
