Re: [PATCH 06/14] hash: add a function to look up hash algo structs

[Junio C Hamano <gitster@pobox.com>]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> In C, it's easy for us to look up a hash algorithm structure by its
> offset by simply indexing the hash_algos array.  However, in Rust, we
> sometimes need a pointer to pass to a C function, but we have our own
> hash algorithm abstraction.
>
> To get one from the other, let's provide a simple function that looks up
> the C structure from the offset and expose it in Rust.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  hash.c      |  5 +++++
>  hash.h      |  1 +
>  src/hash.rs | 15 +++++++++++++++
>  3 files changed, 21 insertions(+)
>
> diff --git a/hash.c b/hash.c
> index 81b4f87027..2f4e88e501 100644
> --- a/hash.c
> +++ b/hash.c
> @@ -241,6 +241,11 @@ const char *empty_tree_oid_hex(const struct git_hash_algo *algop)
>  	return oid_to_hex_r(buf, algop->empty_tree);
>  }
>  
> +const struct git_hash_algo *hash_algo_ptr_by_offset(uint32_t algo)
> +{
> +	return &hash_algos[algo];
> +}

Hmph, technically "algo" may be an "offset" into the array, but I'd
consider it an implementation detail.  We have hash_algo instances
floating somewhere in-core, and have a way to obtain a pointer to
one of these instances by "algorithm number".  For the user of the
API, the fact that these instances are stored in contiguous pieces
of memory as an array of struct is totally irrelevant.  For that
reason, I was somewhat repelled by the "by-offset" part of the
function name.

The next function ...

>  uint32_t hash_algo_by_name(const char *name)

... calls what it returns "hash_algo", but the "hash_algo" returned
by this new function is quite different.  One is just the "algorithm
number", while the other is "algorithm instance".  Perhaps calling
both with the same name "hash algo" is the true source of confusing
naming of this new function?

> +use std::os::raw::c_void;
> +
>  pub const GIT_MAX_RAWSZ: usize = 32;
>  
>  /// A binary object ID.
> @@ -160,4 +162,17 @@ impl HashAlgorithm {
>              HashAlgorithm::SHA256 => &Self::SHA256_NULL_OID,
>          }
>      }
> +
> +    /// A pointer to the C `struct git_hash_algo` for interoperability with C.
> +    pub fn hash_algo_ptr(self) -> *const c_void {
> +        unsafe { c::hash_algo_ptr_by_offset(self as u32) }
> +    }
> +}
> +
> +pub mod c {
> +    use std::os::raw::c_void;
> +
> +    extern "C" {
> +        pub fn hash_algo_ptr_by_offset(n: u32) -> *const c_void;
> +    }
>  }

I am somewhat surprised that we do not expose "struct git_hash_algo"
the same way a previous step exposed "struct object_id" in C as
"struct ObjectID" in Rust, but instead pass its address as a void
pointer.  Hopefully the reason for doing so may become apparent as I
read further into the series?