Skip to content
New issue

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

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

Already on GitHub? # to your account

Add support for custom allocator #1100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edrevo
Copy link
Contributor

@edrevo edrevo commented Nov 22, 2024

Add support for the GIT_OPT_SET_ALLOCATOR option.

/// This allocator will then be used to make all memory allocations for
/// libgit2 operations. If the given `allocator` is None, then the
/// system default will be restored.
pub unsafe fn set_allocator(
Copy link
Member

Choose a reason for hiding this comment

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

For an unsafe fn, we'll need to put a comment here for callers to know how to use it safely

Copy link
Member

@weihanglo weihanglo Nov 22, 2024

Choose a reason for hiding this comment

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

Alternatively, we don't provide this unsafe binding. People can use libgit2-sys directly at their own risks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a fair point. Would it be better to take an std::alloc::Allocator as an input?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, though that is not what I was looked at. I was more on something like, could I call it multiple times? Could I reset after I allocated something in a custom allocator?

reset_search_path has this comment"

This function is unsafe as it mutates the global state but cannot guarantee
thread-safety. It needs to be externally synchronized with calls to access
the global state.

Do we have any considerations on the unsafety of this function?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants