Skip to content

Conversation

gbaraldi
Copy link
Contributor

GMP supports custom allocators so the correct thing is to respect their allocation functions

@@ -123,9 +124,11 @@ inline void mp_set_str(integer_class &i, const std::string &a)

inline std::string mp_get_hex_str(const integer_class &i)
{
void (*freefunc)(void *, size_t);
mp_get_memory_functions(NULL, NULL, &freefunc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? Would mpz_clear not use any custom deallocator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe mpz_clear is only for mpz_t instances, because mpz seems to want to track how much memory was allocated/freed.
https://gmplib.org/manual/Converting-Integers and https://gmplib.org/manual/Custom-Allocation.
It might work, but I'm not sure how correct it would be, since they go through different paths inside of gmp.
https://github.com/alisw/GMP/blob/2bbd52703e5af82509773264bfbd20ff8464804f/gmp-impl.h#L715-L716

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I misunderstood the situation. This is for the string allocation not for mpz_t

Copy link
Contributor

@rikardn rikardn left a comment

Choose a reason for hiding this comment

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

I think this looks good. One future optimization could be to have the free function in a global variable to not have to ask for it every time.

@rikardn rikardn merged commit d21cfe1 into symengine:master Nov 21, 2022
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