On 2024.09.10 12:04, Calvin Wan wrote:
> On Mon, Sep 9, 2024 at 1:47 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> >
> > Junio C Hamano <gitster@xxxxxxxxx> writes:
> >
> > >> +struct libgit_config_set *libgit_configset_alloc(void)
> > >> +{
> > >> + return git_configset_alloc();
> > >> +}
> > >
> > > git_configset_alloc() returns "struct config_set *" while this thing
> > > returns an incompatible pointer.
> > >
> > > Sent out an outdated version or something? This wouldn't have
> > > passed even a compile test, I suspect.
> >
> > The "shim" layer should hide the details of interfacing to the git
> > proper from callers, as well as it should hide the callers' from the
> > git proper. So if you really want to hide "struct config_set" from
> > your library callers, you may need to do something like the
> > attached, perhaps? At least this does pass compilation test.
>
> You're correct that this is supposed to be the shim layer, where
> callers aren't supposed to be able to directly interface with the
> config_set struct. From Rust's perspective, all pointers that come
> back from C code are effectively void* so the naming doesn't make a
> functional difference for our use case (and consequently these
> warnings did not show up from the build.rs script nor did the tests
> fail). However, I agree that the public interface should pass the
> compilation test and your approach does that -- will reroll with those
> changes and I believe that we should also fix the build.rs so that
> warnings also show up during cargo build.
Yeah, cargo will complain if DEVELOPER=1 is set. We should probably
enforce that in build.rs.
