On 2024.08.08 13:18, Kyle Lippincott wrote: > On Thu, Aug 8, 2024 at 11:22 AM Josh Steadmon <steadmon@xxxxxxxxxx> wrote: > > > > On 2024.08.07 23:55, brian m. carlson wrote: > > > On 2024-08-07 at 23:05:00, Josh Steadmon wrote: > > > > Yeah, needing to free() is the only thing we striclty need from libc > > > > right now. Please correct me if I'm wrong, but IIUC then any memory that > > > > is allocated on the C side and then passed to Rust needs one of: > > > > 1) freed by libc::free() on the Rust side, > > > > 2) passed back to the C side to be freed there, or > > > > 3) leaked > > > > > > > > Am I correct in assuming that your opinion is that writing additional > > > > *_free() functions on the C side is worth it to avoid libc? If so, then > > > > I'm fine with including that in V2. > > > > > > I think if we're going to be writing a general purpose API for > > > libification, we probably should provide free functions. Normally, that > > > will be a call to free(3) > > > > [snip] > > > > So in this case, does that mean we'd replace our call to `libc::free()` > > with just `free()`, and then add a declaration for `free` in our > > `extern "C"` section of cgit-sys? It seems to work on my machine, but is > > that actually the more portable option compared to using libc::free? Or > > have I misunderstood something? > > I think both having a generic 'free' function, or requiring your API > consumer to have a compatible 'free' function is undesirable. If the > API hands you something that you must return/free, there should be a > function for that specifically. So I would expect if the API has a > `libgit_foo_get(foo** f)` function, there'd be a paired > `libgit_foo_release(foo* f)` (ignoring whatever squabbles we want to > have about the names). Requiring `libgit_foo_get(foo** f)` to be > paired with `libc::free(f)` limits us to always using libc malloc; > pairing it with `libgit_free((void*)f)` means we can't refcount it / > ignore it if it's a part of a parent object, etc. Sorry, the diff context got removed, the the particular case I was talking about here is where we get back a strdup()ed string from some of the config API calls. I guess we could add a c_str_free() somewhere on the Git side and call that, but it seems like a bit of overkill. I do agree with you about providing _free() [or in our case, _clear_and_free()] functions for more complicated data types though.
