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

Expose mem allocator on C-API #1983

Merged
merged 4 commits into from
Apr 10, 2016
Merged

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Apr 6, 2016

Combining #1973 and #1821

Adds the following new C-API functions:

  • void* sass_alloc_memory(size_t size)
  • char* sass_copy_c_string(const char* str)
  • void sass_free_memory(void* ptr)

IMO this should be future proof and both linked issues should be covered.

  • Update documentation
  • Delete obsolete interface
  • Get rid of Sass::sass_strdup

I suggest to add them with #1974 to 3.3.5 as incubating features for 3.4.0.

//CC @xoofx, @am11, @drewwells

@mgreter mgreter changed the title Expose feature api mem allocator Expose mem allocator on C-API Apr 6, 2016
@mgreter mgreter added this to the 3.3.5 milestone Apr 6, 2016
@mgreter mgreter self-assigned this Apr 6, 2016
@xoofx
Copy link
Contributor

xoofx commented Apr 6, 2016

ship it! 😅

@xoofx
Copy link
Contributor

xoofx commented Apr 6, 2016

Also a broader plan would be to replace all internal malloc/free so that they go through this API to allocate things (and internally also all C++ objects would use the same allocator), so that custom mem allocator could then be pluggable.

@am11
Copy link
Contributor

am11 commented Apr 7, 2016

LGTM - Thanks @mgreter ! 🎉

@mgreter mgreter force-pushed the feature/C-API-mem-allocator branch 3 times, most recently from 5f58e50 to 38fc2a7 Compare April 7, 2016 22:14
@mgreter mgreter force-pushed the feature/C-API-mem-allocator branch 2 times, most recently from 5422da5 to 47d2cab Compare April 8, 2016 00:19
@mgreter mgreter force-pushed the feature/C-API-mem-allocator branch from 47d2cab to 913150f Compare April 8, 2016 00:39
@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 79.184% when pulling 913150f on mgreter:feature/C-API-mem-allocator into aa8aa2d on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented Apr 10, 2016

I plan to release 3.3.5 this week. I'd do it sooner but I'm at a conference.

@mgreter can you please get started on some release notes for this change?

@am11
Copy link
Contributor

am11 commented May 8, 2016

@mgreter,

Adds the following new C-API functions:
• void* sass_alloc_memory(size_t size)
• char* sass_copy_c_string(const char* str)
• void sass_free_memory(void* ptr)

can we ADDAPIfy these three signatures as well, so they are visible for FFI (P/Invoke)? Currently dumpbin /exports libsass.dll does not list them.

At the moment I am using local patch to make it work:

 // to allocate buffer to be filled
-void* sass_alloc_memory(size_t size);
+ADDAPI void* ADDCALL sass_alloc_memory(size_t size);
 // to allocate a buffer from existing string
-char* sass_copy_c_string(const char* str);
+ADDAPI char* ADDCALL sass_copy_c_string(const char* str);
 // to free overtaken memory when done
-void sass_free_memory(void* ptr);
+ADDAPI void ADDCALL sass_free_memory(void* ptr);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants