Regression between 3.1.0 and 3.2.0. sass_compile_data_context now *requires* heap allocated c-strings

[asottile]

This makes it really awkward for things allocated in other ways (stack, borrowed pointers, etc.).

This also seems to have regressed between 3.1.0 and master in this commit: 3187a3c

My main concern is it is going to make the python bindings a little clunky. I used to be able to do this: https://github.com/dahlia/libsass-python/blob/python/pysass.cpp#L435 , but to implement it for 3.2.0 I'll need to malloc + copy + call (not to mention I'll have mismatched malloc() and delete[] since I can't really alloc with new[] in C).

[mgreter]

I don't really understand your concerns. It is indeed a breaking change as documented in the release notes (related issue #925). This actually only means that you have to pass libsass a copy of the source, it you are not able to tell you garbage collector, that you've given the control over to someone else!

To be clear, you have to call strdup if available, or you can use sass_strdup from libsass (if using c++, not sure about c). If you don't have them, yes, you need to implemented a function with malloc and friends. So if you have a string in c, you now have to pass strdup(string.c_str()). IMHO not really that much of a concern. sass/node-sass#764 and sass/perl-libsass@589d490#diff-5b390435e30d16e259818ad6b65d1dafR651

OK, saw your example code and I guess that is all you need sigh

context = sass_make_data_context(strdup(string));

[asottile]

How can I avoid the undefined behaviour introduced by mismatched malloc() + delete[]: http://stackoverflow.com/a/2570224/812183

[mgreter]

Don't allocate via c++ (new), just pass memory allocated via malloc or any other function you know the semantics of. I don't know any other way, but the sources vector has this contraint, since it frees (not delete) the memory is has assigned (meaning it has to be allocated via malloc before, and not via new). The easiest fix seems to make a copy, as with a c++ string: strdup(string.c_str()), to be sure to comply to this constraint. Actually we get reports in whatever direction we go, but since it is a char* pointer, the C semantics for memory allocation seem to make sense!

[asottile]

Couldn't the strdup call be moved into sass_make_data_context? Avoiding transferring of ownership would be ideal :)

[mgreter]

Sorry but a clear no here, because we want to allow optimal performance. Why should you want to make a copy if you only loaded it for libsass. It might be minimal, but that what the interface always was designed for (this would then need to change a lot of char* to const char*). It was an error on our side to make the copy in libsass 3.1. I don't see why this is such a big issue, or asked otherwise, why can't you give libsass control over your loaded memory (do you really still need it -> make a copy)?

[asottile]

I can't give control because I'm borrowing the reference myself (from a PyObject* bytes object which is being refcounted and managed by the python runtime gc mechanism). I still think it's a bit of a shame to trade performance for UB, but I'd understand if you'd rather not change it. const char* would be even better though :)

[mgreter]

Still not sure what the problem is, before libsass made the copy (always), now you can decide if you can move the memory to libsass and if not, you now have to make the copy yourself! Nothing more, nothing less! So in your situation you either need to tell your GC that it is no longer in control of that memory or you simply create a copy (which again, before libsass was always doing a copy for you, so disallowing the move of memory, which is now possible)!

[asottile]

I'm going to make a copy, but if the library was making the copy it could control how it was copied and avoid malloc / delete[] mismatch. Not a really big deal though, I'll live :)

[saper]

I see this change has caused some concern :) It is actually very rare for C libraries to accept pointers form callers and free them.

And unfortunately copying strings is not going to help performance, first because copying takes time, second (more important) that compiler can speed up the code if it knows pointers are const, as suggested above.

Looking at the commits dealing w/the problem I have some ideas:

• rename Context::sources to Context::heap_sources, since it is used only to deallocate memory in the destructor.
• Introduce Context::add_heap_source to be used by functions using resolve_and_load() as well as isass2scss conversion. This function would do heap_sources.push_back(contents) and then call original add_sources(). The latter would need to have sources.push_back() removed of course. That would reversing the effect of 3187a3c (be explicit about heap-allocated strings owned by libsass instead of copying user-supplied strings to make sure they are heap allocated).
• Regarding 7e1b94b I am not sure why should sass_option_set_##option setters free old values. I think nothing in the libsass code allocates there...
• sass_option_push_include_path does not need to strdup() and it should not be freed as well. include_paths handling seems to have some redundancy but I don't see a case where we need to heap-allocate a string there from within libsass.
• I think something could be done with the error text buffers, so you would not need strdup() or any other utility function at all...