[RFC] Refactor/Finalize/Encapsulate C Interface

[mgreter]

In reference to #79, sass/sassc#26

We are getting more false bug reports because people do not use the interface correctly. I guess this is mostly due to lack of documentation. Some parts of the API also feel pretty clumsy, as we have to set the same options for file_context and data_context separately.

Therefore I propose quite a radical refacor, which I already have tested in a local branch. This will mean that bindings will once again have to fallow suit, but I really hope this would be the last time! I also did that already for the perl-libsass bindings (took me about 1 hour, and the code now looks much cleaner and leaner).

I have taken apart the c context structs into one more levels than before.

struct sass_options;
struct sass_context;
struct sass_file_context;
struct sass_data_context;

I have added a wiki page with documentation, in the hope it will add more value to this API refactoring.

Best regards

[bdkjones]

This is incredibly useful and was long overdue. The sass_interface file has never been really well documented. A few comments there would go a LONG way towards helping implementers.

[mgreter]

IMO there is still some place for improvement with the include paths :)
I also just found that there is another IMO obsolete api to compile in sass.cpp!
Removing it completely did not have any impact in perl-libass? Maybe someone can clarify!?

[craigbarnes]

I'm not so sure about the self-generating header declarations though. Some languages have FFI libraries that can parse header declarations but completely choke on non-trivial macros.

[mgreter]

I'm pretty sure there are other places (like ast.cpp) where more complex makros are used.
I just confirmed that it would be possible to move the complete C API to sass.h.
IMO this would be the logical header if there was a shared libsass.so around!

[craigbarnes]

Macros aren't problematic per se, just the ones in public headers. The public headers are usually the only thing parsed by FFI libraries, binding generators, etc.

[mgreter]

@QuLogic maybe you could clarify this for me. I guess I have to return a pointer from all the make_sass_value functions. Which means I'll have to allocate the memory on the heap (which I guess makes a lot of sense, since this is what makes it very robust, as we only pass around a pointer, correct?). Otherwise I don't seem to be able to move all the structs to the main file.

union Sass_Value make_sass_boolean (int val);

[QuLogic]

Yea, that definition will return the union by value, which means the stuff from the structure needs to be copied from the function's copy to the caller's copy. That means the compiler needs to know the actual size of it. Using a pointer would reduce that copying to just the pointer (which may fit in a register even).

You could do:

typedef union Sass_Value *Sass_Value;

in the header and then just use Sass_Value everywhere. As long as you don't have to dereference the union in the header, there should be no need for the full layout.

[mgreter]

I made some progress and was able to move all structs and other implementation details into sass.cpp, while only leaving functions that deal with pointers in sass.h. There are IMO some minor flaws with how the specific Sass_Values have to be accessed. I guess this area leaves some room for more improvement! Beside that the code in perl-libsass is much better readable with this!

IMO I could change all setters/getters for Sass_Values to accept the generic value, instead of needing the fetch the specific struct first. But this may lead to crashes if people are passing it the wrong Sass_Value type. But the same applies to getters/setter for list values if you give it an index that is out of bound. So the question is if this is acceptable, as the user is able to check the length or type before calling the function? I could also add safe guards and do the checks everytime, but I do not like this idea!

old: sass_color_get_r(struct Sass_Color* v);
new: sass_color_get_r(struct Sass_Value* v);

I'm not yet sure if I should go for a typedef for Sass_Values. IMO it does not work to use the same name as @QuLogic suggested. I would probably need to create other names. But libsass seems to hide the pointer for Sass_C_Function too, so it might be a good idea to follow suit.

[QuLogic]

That make_sass_null should be (void).

[mgreter]

Next update

Re-added sass_interface to support the old API side by side with the new API. IMO this will make the transition much easier for most bindings (ie. node-sass). For now I have splitted up the functionality into seperates headers and code files.

sass_values - Sass_Value implementation
sass_functions - Sass_C_Function_Descriptor
sass_context - New context interface
sass_interface - Old context interface

I have also creates a separate sass_structs.h or should other internal code also only access the structs via the API functions? Well, there is just context.cpp that is still accessing the options directly. It should be trivial to move values.h and functions.h back to sass.h. Once the old sass_interface is gone, the rest could also be moved there!

There is no other place than sass_context that uses the struct directly, and that is exactly where it belongs :)

[mgreter]

I got everything ready for a PR in another local branch.

• Hide all implementation details in the new C API defined in sass_context.h
• Split up Sass_C_Function, Sass_Value and Sass_Context in their own code files. This means we also have more (system) header files than before, but IMO that doesn't really matter.
• Added new API beside the old. Include sass_interface.h for the old; sass_context.h for the new
• New API context structs "inherit" from each other (unfortunately I was not able to forward declare this inheritance without exposing the struct again, should be possible IMHO). But you can cast them down youself or use the accessor functions of the API (recommended).
• Removed automatic inclusion of sass2scss.h
• Added "load only once" guards inside the header files (why is this not done with all header files?)
• Normalized api struct names to camel case to match Sass_Value and Sass_C_Function
• Added typedefs for Sass_C_Function_List and Sass_C_Function_Callback

If the implementor does not use C_Functions and Sass_Values it should work as before.
This should probably also improve memory usage, as Sass_Value is now only a pointer.

Will need to update the docs before doing the PR!
Looking into returning error status plus better include path handling now!

Hope you guys like it!

//CC @am11 Would you be willing to try to compile it against node-sass?

[am11]

@mgreter, great work refactoring interface! 👍

I will definitely give it a shot. Unfortunately, we don't have functions implemented yet. But rest of it can be absolutely tested out! :)

[mgreter]

I basically meant that it should still compile as before if you do not use functions or values in the binding! Would be interesting to know if this is really the case! Beside that feel free to try to adapt node-sass to use the new interface, but I'll add some more documentation once it is finished :)

[HamptonMakes]

Wait, we have a generic Sass function callback?

[am11]

@mgreter, unfortunately, it is failing with GCC and VCC.

For GCC errors, see https://travis-ci.org/am11/node-sass/jobs/40434617#L353-L510.

For VCC see http://pastebin.com/zMrvajsv.

[mgreter]

Hmm, strange, will try on linux, as I can get this one running :)
By the way, sassc builds fine, but not sure if it is actually using sass_interface!

@hcatlin : Not sure what you mean by "generic", but yeah, there is the possibility to hook functions into libsass pretty easy via C.

[QuLogic]

@mgreter I haven't had a chance to look at what you did, but just based on the log, it looks like you need to include a header to get size_t in sass_values.h. Also, I think you don't need quite so many const. I don't think it makes too much sense for a return of an integral type, and there are quite a few warnings about it.

@am11 Complete side note, but you shouldn't have to run g++ version through sudo.

[HamptonMakes]

@mgreter generic as in, it functions like method_missing and unresolved function call attempts during execution are passed to the callback dynamically. Aka, they don't have to be defined (named) at compile time, and we could write a full RubySass compatibility layer (you could write Sass functions in Ruby)

[HamptonMakes]

The more limited (aka, must be predefined) function callback system was supposed to get ripped out and replaced with one that functions as a function resolver.

[mgreter]

@QuLogic I was thinking about the consts and to be honest I am not really sure where they make sense and where not. Maybe you could give some pointers I could fallow?

@hcatlin It actually was ripped out with a refactoring about a year ago, I added them back (also about a year ago) since perl-libsass already supported them (and they are IMO quite usefull). But a more generic solution seems even better (I guess this could even co-exist). I may have a look and see how difficult it would be to add something (I already wanted to check if overloading imports etc. would be feasable).

[am11]

Here is one mysterious looking error from VCC:

c:\users\adeel\source\repos\node-sass\src\libsass\inspect.hpp(98): error C2664: 'void Sass::Inspect::fallback_impl(Sass
::AST_Node *)' : cannot convert argument 1 from 'Sass::Selector_List *' to 'Sass::AST_Node *' (..\src\libsass\sass.cpp)
 [C:\users\adeel\source\repos\node-sass\build\binding.vcxproj]
          Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

Would it make sense to use reinterpret_cast() in that context for clarity and shutting up the VC compiler. 😎

[akhleung]

@mgreter @hcatlin I think the best solution in the long run would be to use dlopen (and whatever the analogous feature is on Windows) to have truly dynamically loadable 3rd party functions, without needing to recompile LibSass.

[QuLogic]

@mgreter Well, in C, I don't think it's as necessary as C++. It makes no real difference for integral types, and there are no classes. It really only makes sense with pointers, e.g., const char * (meaning a pointer to a constant character string).

The usual contract is const means the code that gets it doesn't own it (i.e., a function with a const argument would copy the argument if it wanted to hang on to it and also would not free the argument, with similar ideas for the return value).

[mgreter]

@QuLogic I guess -Wextra pointed me in the same direction!
I am now checking if I can get node-sass to compile on linux!

[mgreter]

@am11 Got it compiled on linux, was just some header missing as @QuLogic pointed out correctly!

make: Entering directory `/root/node-sass/build'
  CXX(target) Release/obj.target/binding/src/binding.o
  CXX(target) Release/obj.target/binding/src/sass_context_wrapper.o
  CXX(target) Release/obj.target/binding/src/libsass/ast.o
  CXX(target) Release/obj.target/binding/src/libsass/base64vlq.o
  CXX(target) Release/obj.target/binding/src/libsass/bind.o
  CC(target) Release/obj.target/binding/src/libsass/cencode.o
cc1: warning: command line option '-std=c++11' is valid for C++/ObjC++ but not for C [enabled by default]
cc1: warning: command line option '-std=c++11' is valid for C++/ObjC++ but not for C [enabled by default]
  CXX(target) Release/obj.target/binding/src/libsass/constants.o
  CXX(target) Release/obj.target/binding/src/libsass/context.o
  CXX(target) Release/obj.target/binding/src/libsass/contextualize.o
  CXX(target) Release/obj.target/binding/src/libsass/copy_c_str.o
  CXX(target) Release/obj.target/binding/src/libsass/error_handling.o
  CXX(target) Release/obj.target/binding/src/libsass/eval.o
  CXX(target) Release/obj.target/binding/src/libsass/expand.o
  CXX(target) Release/obj.target/binding/src/libsass/extend.o
  CXX(target) Release/obj.target/binding/src/libsass/file.o
  CXX(target) Release/obj.target/binding/src/libsass/functions.o
  CXX(target) Release/obj.target/binding/src/libsass/inspect.o
  CXX(target) Release/obj.target/binding/src/libsass/json.o
  CXX(target) Release/obj.target/binding/src/libsass/node.o
  CXX(target) Release/obj.target/binding/src/libsass/output_compressed.o
  CXX(target) Release/obj.target/binding/src/libsass/output_nested.o
  CXX(target) Release/obj.target/binding/src/libsass/parser.o
  CXX(target) Release/obj.target/binding/src/libsass/prelexer.o
  CXX(target) Release/obj.target/binding/src/libsass/remove_placeholders.o
  CXX(target) Release/obj.target/binding/src/libsass/sass.o
  CXX(target) Release/obj.target/binding/src/libsass/sass2scss.o
  CXX(target) Release/obj.target/binding/src/libsass/sass_values.o
  CXX(target) Release/obj.target/binding/src/libsass/sass_functions.o
  CXX(target) Release/obj.target/binding/src/libsass/sass_interface.o
  CXX(target) Release/obj.target/binding/src/libsass/sass_util.o
  CXX(target) Release/obj.target/binding/src/libsass/source_map.o
  CXX(target) Release/obj.target/binding/src/libsass/to_c.o
  CXX(target) Release/obj.target/binding/src/libsass/to_string.o
  CXX(target) Release/obj.target/binding/src/libsass/units.o
  CXX(target) Release/obj.target/binding/src/libsass/utf8_string.o
  CXX(target) Release/obj.target/binding/src/libsass/util.o
  SOLINK_MODULE(target) Release/obj.target/binding.node
  SOLINK_MODULE(target) Release/obj.target/binding.node: Finished
  COPY Release/binding.node
make: Leaving directory `/root/node-sass/build'
gyp info ok

Just don't forget to add the new source file sass_values.cpp and sass_functions.cpp ;)
By the way, how do I run the test suite once I got it compiled??

@QuLogic dropped most of the consts (only left the ones for char*)!

[QuLogic]

  CC(target) Release/obj.target/binding/src/libsass/cencode.o
cc1: warning: command line option '-std=c++11' is valid for C++/ObjC++ but not for C [enabled by default]
cc1: warning: command line option '-std=c++11' is valid for C++/ObjC++ but not for C [enabled by default]

For this one, -std=c++11 should be in CXXFLAGS not CFLAGS/CPPFLAGS? (Not sure how you're compiling it.)

[mgreter]

I added the following includes to all C headers.
Beside them no other system headers are used!

#include <stddef.h>
#include <stdbool.h>

For the node-sass warning see here.

[mgreter]

Updated the documentation wiki page for a better overview!

[mgreter]

And added json error reporting for the new context API! //CC @am11

[mgreter]

@hcatlin Regarding generic callback function:
Got some time left and implemented a generic callback for unknown functions.
Unfortunately I was not able to prepend the original function name to the arguments!

In my bindings I can now define a function named * to get called for all unknown functions!
IMO we could use this interface also to overload @import and @warn statements!?

[mgreter]

Next commit adds possibility to overload @warn statements.
Instead of printing to cerr a custom function is called instead!

[am11]

@QuLogic, thanks for the tip. Actually, I added couple of statements in .travis.yml to update g++ and gcc. I understand sudo everything is kind of an anti-pattern, but it is the one which always work! 😄

@mgreter, thanks! It built and passed both tests. https://travis-ci.org/am11/node-sass/builds/40448866. While on windows, it is still choking with handful of reinterpret_cast messages. Normally, such things are thrown as warnings in VC++ paradigm. But this is C in Windows, so you never know. 😜
Can you enlighten me with the usage of JSON error reporting? 👍
Does it also serialize exception messages as JSON?

[am11]

darrenkopp/sassc-win and darrenkopp/libsass-net stopped working.

BTW, any thoughts on splitting sassc-win test code for Appveyor? Currently, we have to update submodule to run the test.

[am11]

To run the tests on node-sass, following is the sequence:

# add remote and get fresh code
# git remote add node-sass https://github.com/sass/node-sass
git pull --rebase node-sass master

# install npm, fetches all the dependences and libsass prebuild binaries
npm install

# to build binaries manually and making sure that you are testing against
# latest binary (not the prebuild ones)
node scripts/build -f

# to run the test
npm test

[HamptonMakes]

Definitely, overloads are one of the most requested features.

This branch is crazy! Guessing we call this 3.1 ;)

[mgreter]

Thanks for all the feedback. If no one objects I'm going to create a PR now!

@am11 For the errors in json, please see this commit here:
mgreter@584932c#diff-9bb0436549f98af72f08f10e1695a7caR256

I also tried to overload import, but that seems to be a bit more difficult as this is happening in parser, which does not seem to have access to env, where the custom functions are stored. Otherwise it would probably have been trivial to add import overloading!

[am11]

@mgreter, it is not exposed in sass_interface.h, then how would we utilize it? Shouldn't there be an option like bool return_errors_as_json (default: false)?

[am11]

Also, does it now capture column info as well (in addition to message, line and file)? (-8

[mgreter]

You have to use the new API then (or you port that part back to sass_interface yourself).
Sorry, but I have to give some incentive to switch over to the new API :)

[am11]

Using your API documentation, https://github.com/mgreter/libsass/wiki/API-Documentation, I will try to refactor node-sass' bindings with new API in a separate branch and wait for libsass to get a tagged release, before merging into node-sass. :)

[mgreter]

IMO we can/will keep the old libsass_interface around for some time!
This should ease the process of getting all implementors (sassc) to the new API!
The option name have all stayed the same. Just replace sass_interface.h with sass_context.h.
Then it should not be very hard to update to the new API, mostly just do what the compiler says :)
Most importantly you need to replace all direct access to structs with one of the new function calls.
Would be cool if you could document your problems and findings to create some "API upgrade" doc!

[mgreter]

Pull request has been placed: #635
Please give your thumbs up if you like to have this merged!

Sorry to also include the other commits, but I'm too lazy right now to take them apart ...

[bdkjones]

Hmm... I'm not really a fan of using a ton of functions just to set options on a struct. That's a lot of wasted overhead. I think a better approach would be to implement a single function that returns a sass_options struct initialized with sane defaults. Then you could modify that struct yourself to customize it. I don't really see myself switching to this new API, so perhaps keeping the old one around is a good idea.

Sent from my iPhone

On Nov 9, 2014, at 13:10, Marcel Greter notifications@github.com wrote:

Pull request has been placed: #635
Please give your thumbs up if you like to have this merged!

Sorry to also include the other commits, but I'm too lazy right now to take them apart ...

—
Reply to this email directly or view it on GitHub.

[bdkjones]

Also, there's really only a handful of implementors that are going to use this API, right? sassc, myself and maybe a few others. All the higher-level implementors are just going to call sassc, no? I think the old sass_interface API was fine, it just needed some solid documentation and comments to make sure folks used it correctly and avoided the common pitfalls. So creating a totally fail-safe API at the expense of performance (no matter how small the decline in performance) isn't a good deal in my opinion. Faster is better. Finally: I've only looked at the documentation on my iPhone on the road, so my apologies if I've missed something.

Sent from my iPhone

On Nov 9, 2014, at 13:10, Marcel Greter notifications@github.com wrote:

Pull request has been placed: #635
Please give your thumbs up if you like to have this merged!

Sorry to also include the other commits, but I'm too lazy right now to take them apart ...

—
Reply to this email directly or view it on GitHub.

[QuLogic]

I'm not really a fan of using a ton of functions just to set options on a struct. That's a lot of wasted overhead.

Is it really? GLib and GTK+ did this for every structure they had, and they don't seem to be complaining.

I think a better approach would be to implement a single function that returns a sass_options struct initialized with sane defaults.

There's no reason there couldn't be helper functions like this alongside the lower-level getter/setters.

Also, there's really only a handful of implementors that are going to use this API, right? sassc, myself and maybe a few others. All the higher-level implementors are just going to call sassc, no?

No, absolutely not. Everyone (binding-wise) should use the API. Shelling out to sassc is a going to lead to a very broken and possibly stagnant interface.

[mgreter]

So creating a totally fail-safe API at the expense of performance (no matter how small the decline in performance) isn't a good deal in my opinion

The API is only used to setup the context and has no influence on the libsass compilation speed.

[mgreter]

@QuLogic Maybe you could explain why this leads to a more robust ABI? I _ guess_ I know why, but some clarification would be greatly appreciated (specially regarding libtool)!

[bdkjones]

Fair enough. I'm fine with the getter/setters as long as I don't have to call all 15 of them just to set the options every time I compile a file. A helper that returns an option struct with initialized defaults would be perfect. As for performance: my general policy is to avoid things that degrade it if at all possible. I know that the additional function call overhead won't make a noticeable difference in speed, but over time, if more and more changes add a little extra overhead, it can add up!

Sent from my iPhone

On Nov 9, 2014, at 14:05, Elliott Sales de Andrade notifications@github.com wrote:

I'm not really a fan of using a ton of functions just to set options on a struct. That's a lot of wasted overhead.

Is it really? GLib and GTK+ did this for every structure they had, and they don't seem to be complaining.

I think a better approach would be to implement a single function that returns a sass_options struct initialized with sane defaults.

There's no reason there couldn't be helper functions like this alongside the lower-level getter/setters.

Also, there's really only a handful of implementors that are going to use this API, right? sassc, myself and maybe a few others. All the higher-level implementors are just going to call sassc, no?

No, absolutely not. Everyone (binding-wise) should use the API. Shelling out to sassc is a going to lead to a very broken and possibly stagnant interface.

—
Reply to this email directly or view it on GitHub.

[bdkjones]

Yep, but setting up the context is part of running Libsass. I definitely understand that this isn't part of the actual compiling step, but it is still a step in the whole process and will be slightly slower than it used to be under the old API. So setting up Libsass will be slightly slower. (I'm also a performance nazi... I may be in the minority.)

Sent from my iPhone

On Nov 9, 2014, at 14:07, Marcel Greter notifications@github.com wrote:

So creating a totally fail-safe API at the expense of performance (no matter how small the decline in performance) isn't a good deal in my opinion
The new API is only used to setup the context. This has absolutely no influence on the libsass compilation speed.

—
Reply to this email directly or view it on GitHub.

[QuLogic]

@mgreter Yes, good point, I was debating whether to mention that because it can be long to explain...

Basically, it means the underlying implementation is uncoupled from the API. If downstream users have access to the structure fields, those fields must stay the same forever (or at least until you create a 4.0.0 or something, and explicitly notify people). Theoretically, if you wanted to add stuff, you couldn't, because a downstream user might be allocating the structure themselves, and when libsass code goes to access the new field, you get a segfault because the structure is not big enough. (I don't think that has stopped libsass developers before, but I chalk that up being more "beta" before. Now that many people use it, it's more important to avoid that.)

Once the structure fields are hidden, all the downstream user has to worry about is a pointer. They can't accidentally allocate it too small, or forget to initialize a new field, or whatever... The libsass developers are free to change the underlying setup entirely, e.g., instead of a structure, it could actually just be a pointer to the C++ class. But downstream doesn't have to know or care. They stay happy as long as nothing's removed from the API, i.e., it gives you forward compatibility.

[mgreter]

My guess was, that if we add more stuff (functions) to the header and that library get installed over an old one in the system lib folder, software that was built against the old library (dynamically linked), will still work/link happily with the newly installed one. Correct? IMO this would also be true if we always added stuff the end end of the structs, but not if something got added in between ...

[QuLogic]

My guess was, that if we add more stuff (functions) to the header and that library get installed over an old one in the system lib folder, software that was built against the old library (dynamically linked), will still link happily with the newly installed one. Correct? IMO this would also be true if we always added stuff the end end of the structs, but not if something got added in between ...

You are correct, except in the case where someone allocates the structure themselves. Then the field you added may or may not be initialized. In fact, the structure may not even be big enough to contain the new fields.

[mgreter]

Good point with uninitialized structs because of wrong initialization.
I remember one bug report that was caused by this missuse of the API.

[mgreter]

Thanks for all the Feedback!
I have merged the changes into master branch!