Re: [PATCH v3 06/15] ivec: create a vector type that is interoperable between C and Rust

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

"Ezekiel Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ezekiel Newren <ezekielnewren@gmail.com>
>
> Trying to use Rust's Vec in C, or git's ALLOC_GROW() macros (via
> wrapper functions) in Rust is painful because:
>
>   * C doing vector things the Rust way would require wrapper functions,
>     and Rust doing vector things the C way would require wrapper
>     ...
>   * Currently, Rust defines its own 'Vec' type that is generic, but its
>     memory allocator and struct layout weren't designed for
>     interoperability with C (or any language for that matter), meaning
>     ...
>   * Similarly, git defines ALLOC_GROW() and related macros in
>     git-compat-util.h. While we could add functions allowing Rust to
>     ...

All the good reasons any C (or any non-Rust language for that
matter) projects would want to have an interoperability Shim
for their dynamically allocated and grown array-like things.

> To address these issue, introduce a new type, ivec -- short for
> interoperable vector. (We refer to it as 'ivec' generally, though on
> the Rust side the struct is called IVec to match Rust style.) 

I however was hoping by now Rust getting used more widely, somebody
has already created a generic "this is how you make C-array and Rust
vectors interoperate" wrapper that latecomer projects like us can
use without inventing our own.

> +INTEROP_OBJS += interop/ivec.o
> +.PHONY: interop-objs
> +interop-objs: $(INTEROP_OBJS)

What is this phony target used for?  No other targets seem to depend
on this one (I am wondering if we need the latter two lines).

> diff --git a/interop/ivec.c b/interop/ivec.c
> new file mode 100644
> index 000000000000..9bc2258c04ad
> --- /dev/null
> +++ b/interop/ivec.c

I am wondering if this needs a new hierarchy "interop"; shouldn't
the existing "compat" be a good fit enough?  I dunno.

Even though this is a shim to somebody else's code, it still is a
part of our codebase, so our CodingGuidelines for C programs should
apply.  

> @@ -0,0 +1,151 @@
> +#include "ivec.h"
> +
> +static void ivec_set_capacity(void* self, usize new_capacity) {
> +	struct rawivec *this = self;

 - Asterisk sticks to the variable, not type.

 - The opening and closing {braces} for the function body are
   written at the leftmost column on its own line.

 - There should be a blank line between the declarations and the
   first statement.

> +	if (new_capacity == 0)
> +		FREE_AND_NULL(this->ptr);
> +	else
> +		this->ptr = xrealloc(this->ptr, new_capacity * this->element_size);
> +	this->capacity = new_capacity;
> +}
> +
> +void ivec_init(void* self, usize element_size) {
> +	struct rawivec *this = self;
> +	this->ptr = NULL;
> +	this->length = 0;
> +	this->capacity = 0;
> +	this->element_size = element_size;
> +}

I notice that this reintroduces a variable named "this", which was
eradicated in 585c0e2e (diff: rename 'this' variables, 2018-02-14).

I do not think those who want to use C++ compilers on our C code
would not mind "self", so how about doing something like...


	void ivec_init(void *self_, usize element_size)
	{
		struct rawivec *self = self;

		self->ptr = NULL;
		self->len = 0;
		self->capacity = 0;
		self->element_size = element_size;
	}

... perhaps?

> diff --git a/interop/ivec.h b/interop/ivec.h
> new file mode 100644
> index 000000000000..98be4bbeb54a
> --- /dev/null
> +++ b/interop/ivec.h
> @@ -0,0 +1,52 @@
> +#ifndef IVEC_H
> +#define IVEC_H
> +
> +#include "../git-compat-util.h"

As we use -I. on the command line, there is no need to add "../"
here; just writing

	#include <git-compat-util.h>

should be enough.  Also, if this file does not depend on the
services compat-util header provides (and I do not think it does
from a brief look at its contents), it is better not to include it.

Instead, the sources (like ivec.c next door we just saw) should
begin themselves with #include of git-compat-util.h header before
including ivec.h.

> diff --git a/rust/xdiff/src/lib.rs b/rust/xdiff/src/lib.rs
> index e69de29bb2d1..8b137891791f 100644
> --- a/rust/xdiff/src/lib.rs
> +++ b/rust/xdiff/src/lib.rs
> @@ -0,0 +1 @@
> +

If this empty line in an otherwise empty file is absolutely
necessary to make Rust work, then please arrange .gitattributes to
tell git that this file is excempt from the usual blank-at-eof
whitespace rule we use.  If not, remove that unnecessary empty line.

Or perhaps remove the file altogether if nobody looks at it???

In any case, given that our top-level .gitattributes file starts
with

    * whitespace=!indent,trail,space
    *.[ch] whitespace=indent,trail,space diff=cpp
    *.sh whitespace=indent,trail,space text eol=lf
    ...
    *.bat text eol=crlf
    CODE_OF_CONDUCT.md -whitespace
    ...

I think a new rule to cover "*.rs" and perhaps *.toml files right
before rules for each specific file begin would be in order.

Thanks.