On 2024.08.22 10:12, Phillip Wood wrote:
> Hi Calvin
>
> On 21/08/2024 19:46, Calvin Wan wrote:
> > Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
> >
> > > before the function declarations. As I said in my comments on the last
> > > patch I think we'd be better to namespace our types as well as our
> > > functions in this library layer so this can be resued by other language
> > > bindings.
> >
> > Are you suggesting something like "#define libgit_config_set
> > config_set"? I wouldn't be comfortable renaming config_set in git.git
> > just yet until config/config_set can be a standalone library by itself.
>
> I was suggesting[1] adding
>
> struct libgit_configset {
> struct config_set set;
> };
>
> to public_symbol_export.c and rewriting the wrappers in that file to use
> this struct e.g.
>
> int libgit_configset_get_int(struct libgit_configset *cs,
> const char *key, int *dest)
> {
> return git_configset_get_int(&cs.set, key, dest);
> }
>
> In public_symbol_export.h we'd then have
>
> struct libgit_configset;
>
> int libgit_configset_get_int(struct libgit_configset *,
> const char *, int *);
>
> If we want the symbol exports to be useful outside of the rust bindings I
> think we need to namespace our types as well as our functions.
>
> [1]
> https://lore.kernel.org/git/5720d5b9-a850-4024-a1fd-54acc6b15a74@xxxxxxxxx
>
> > > > + pub fn get_str(&mut self, key: &str) -> Option<CString> {
> > >
> > > If we're adding an ergonomic api then having return CString isn't ideal.
> > > I think the equivalent function in libgit2-rs has variants that return a
> > > String which is convinent if the caller is expecting utf8 values or
> > > Vec<u8> for non-utf8 values.
> >
> > Having both get_cstr() and get_str() makes sense to me.
>
> Just to be clear get_cstr() would return Vec<u8>? I'm far from a rust expert
> but my understanding was that crates that wrap C libraries generally avoid
> using CString in their API.
>
> Best Wishes
>
> Phillip
Yeah let's just go with "get_string()" and return an Option<String>.
