Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: exposes allocation functions via C bindings #968

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

NickUfer
Copy link
Contributor

The current C bindings require the caller to allocate memory for new structs, pass it to Manifold, and manually deallocate it using the exposed delete functions. This approach breaks the best practice of allocating and deallocating memory in the same context, especially in the context of FFI.

This PR introduces functions that leverage Manifold's internal allocator to handle memory allocation for each relevant struct. This provides an alternative to the current method of external allocation, while preserving backward compatibility with the existing approach.

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 85.18519% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.21%. Comparing base (d437097) to head (f4c4733).
Report is 113 commits behind head on master.

Files with missing lines Patch % Lines
bindings/c/manifoldc.cpp 65.21% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
- Coverage   91.84%   88.21%   -3.63%     
==========================================
  Files          37       62      +25     
  Lines        4976     8672    +3696     
  Branches        0     1045    +1045     
==========================================
+ Hits         4570     7650    +3080     
- Misses        406     1022     +616     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@elalish
Copy link
Owner

elalish commented Sep 30, 2024

Thanks! The C bindings are not my forte, so I'll be looking to others for review. However, one question - could this be simplified/improved if we didn't need to preserve backward compatibility? I ask because we're making a number of breaking changes right now for v3.0, so if we'd want to eventually, now is a good time.

@elalish
Copy link
Owner

elalish commented Sep 30, 2024

Also, does this fix the issue you found, or only provide a working alternative?

@geoffder
Copy link
Collaborator

Thanks! The C bindings are not my forte, so I'll be looking to others for review. However, one question - could this be simplified/improved if we didn't need to preserve backward compatibility? I ask because we're making a number of breaking changes right now for v3.0, so if we'd want to eventually, now is a good time.

I think that adding the alternative is better than removing the existing option that allows the user to provide their own memory. It allows buffer re-use if that is desired, and depending on the host language it might be less fuss to have it handle the allocation rather than registering something allocated elsewhere. I'm not an expert in how other languages handle FFI, but I've never had problems with the current approach.

@NickUfer
Copy link
Contributor Author

Thanks! The C bindings are not my forte, so I'll be looking to others for review. However, one question - could this be simplified/improved if we didn't need to preserve backward compatibility? I ask because we're making a number of breaking changes right now for v3.0, so if we'd want to eventually, now is a good time.

As @geoffder already mentioned, it is good to have both versions, probably no need to change everything and break compatibility.
I also haven't encountered any problem with the current approach (at least in my environment), but Rust discourages allocation and deallocation in different contexts, so I would like to follow that guidance.

@NickUfer
Copy link
Contributor Author

Also, does this fix the issue you found, or only provide a working alternative?

To also clear this one up: The code I posted was my own testing and wasn't in this project 😄 This was basically the first approach of this PR, where I found out it was leaking memory. This PR contains the fixed version and does not leak anymore.

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this looks pretty safe - thanks! Also, you may have noticed that the testing on our C-bindings is a bit sparse. Feel free to add some, particularly things where you'd like some assurance they keep working as you expect.

@NickUfer
Copy link
Contributor Author

NickUfer commented Sep 30, 2024

For now I replaced the manual malloc + struct size function calls by the new allocaton functions. Under the hood the same calls happen but it now tests the existing and new functions at the same time. So it should increase the coverage.

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, thanks.

@elalish elalish merged commit 5719926 into elalish:master Oct 1, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants