search

OpenTimelineIO / OpenTimelineIO-C-Bindings

C bindings for the OpenTimelineIO Library (http://opentimeline.io)
http://opentimeline.io
Apache License 2.0
11 stars 7 forks source link

Add a function to free allocated memory #52

Closed BadSingleton closed 1 year ago

BadSingleton commented 1 year ago

Managed runtimes like C# P/Invoke requires this in order to be able to correctly free memory in managed code that was allocated by unmanaged code.

The method was only defined in copentime because it can be used on it's own but opentimlineio must be used with opentime.

BadSingleton commented 1 year ago

This is a subset of #15. It doesn't solves the interning of strings, only exposes free so that managed runtimes can free memory from managed-land.

meshula commented 1 year ago

there's some complexity in COpentimelineIO about the memory management of retained objects and retained objects in containers. I'm slightly worried that if we expose something named "free" people might get excited and think that they can use to more easily manage retained COTIO objects, which isn't the case.

I'm thinking naming and documentation would help. Could you maybe in this thread, explain who is going to call the wrapped free and what they are going to be freeing?

BadSingleton commented 1 year ago

Apologies for the delay. This is for managed runtimes like C# who can write all the bindings as managed code. In an example case of C# where the bindings are coded in pure C# via P/Invoke, without an exposed free function, cstrings are a memory leak as managed strings are made by copying the cstring. Ownership by the managed runtime would also be a problem; as when garbage collected, they wouldn't be able to free the memory allocated by the native runtime. You can also see this PR as a very basic subset of #15

meshula commented 1 year ago

I see, that makes sense. Two questions

(1) can you run git commit --amend --signoff on your repo and force push? That will clear up the DCO warning we can see in the CI. We can go ahead and land it at that point to unstuck you.

(2) My gut feeling is that maybe it's time to move ahead with the pattern in #15. Would that allow for anything interesting in your C# bindings, in terms of easier management of strings or whatever?

BadSingleton commented 1 year ago

Would that allow for anything interesting in your C# bindings, in terms of easier management of strings or whatever?

It would make this PR obsolete, which is a good thing

edit: if the otiostr_delete is exported/external visibility in #15, it would obsolete this PR

meshula commented 1 year ago

Cool, thanks!