Refactor C API for more robust ABI

[mgreter]

The C Interace was never really well defined or documented!

There is a very long standing issue open for libsass to improve this.

• 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

Added a documentation wiki page to my local repo!

Since the introduction of autotools build we also can create a libtool system library!

There are also three other features implemented in this PR:

• Add json error reporting for new context api (CLI: Report character position in error #234)
• Implement generic function callback (register as *)
• Implement @warn overloading via custom function (register as @warn) (Way to capture Warnings #539)
• Also contains a bugfix for ctx->includedFiles misleading stdin #634

I know these should probably go in their own PR, but hadn't had time to do it.
If this is a problem I can remove the commits from this PR!

Since it is a pretty big change, I really would like to see some thumbs up before merging it!

[craigbarnes]

I haven't taken a proper look at the new API yet, but I just built lua-sass against this branch (still using the old API) and it builds and passes the tests fine.

[QuLogic]

This is where the function name ends up as the last argument, right? Could you create a new Arguments with the function name and += the original arguments? (I'm assuming you really wanted the function name first from your other comment.)

[mgreter]

I really tried, but I couldn't figure out how I could do it. Underlying it is a Vectorized Object and only the << operator seems overloaded. I tried to access the vector directly to insert it, but there is too much magick going on there. IMO it would feel more natural to prepend the original function name, but adding it to the end is also working (just feels a bit bad). Maybe I'll give it a shot again later!

[QuLogic]

Maybe I'm missing something, but I don't see the << being overloaded by the Arguments class. I do see << in Vectorized, but there's also +=.

[xzyfer]

Arguments extends Vectorized which overloads <<
On 10 Nov 2014 12:38, "Elliott Sales de Andrade" [email protected]
wrote:

In eval.cpp:

@@ -381,8 +407,13 @@ namespace Sass {
}
// else if it's a user-defined c function
else if (c_func) {

•  if (full_name != "*[f]") {
  

•    bind("function " + c->name(), params, args, ctx, &new_env, this);
  

•  } else {
  

•    String_Constant *str = new (ctx.mem) String_Constant(c->path(), c->position(), c->name());
  

•    *args << new (ctx.mem) Argument(c->path(), c->position(), str);
  

Maybe I'm missing something, but I don't see the << being overloaded by
the Arguments class. I do see << in Vectorized, but there's also +=.

—
Reply to this email directly or view it on GitHub
https://github.com/sass/libsass/pull/635/files#r20064005.

[mgreter]

OK, figured it out, I did not find a way to prepend to an existing Arguments list, but I was able to create a new one with first adding the name and then appending the rest of the arguments:

Arguments* new_args = new (ctx.mem) Arguments(c->path(), c->position());
*new_args << new (ctx.mem) Argument(c->path(), c->position(), str);
*new_args += args;
args = new_args;

[mgreter]

Thanks @craigbarnes for trying it out. Good news it is still working with the old interface!

[mgreter]

By the way, the commit with the custom import overloading also slipped through.
Did a lot of basic tests with it, so I guess it can stay as it also is part of the C API.

[QuLogic]

Parens are unnecessary, BTW. * is nearly highest priority, and && is practically the lowest.

[mgreter]

Had some time to merge and clean-up the commit history!
In the spirit of @hcatlin (if I remember correctly), I'm giving this a 👍 ;)

[mgreter]

By the way, I've updated perl-libsass to support the complete new API (incl. importer).
Have not yet added unit tests or documentation there, but might be usefull for some people.
https://github.com/sass/perl-libsass/blob/api/c-interface/lib/CSS/Sass.xs

[mgreter]

Seems like there is always something than can be further improved :)
In the mean time I have adjusted sassc to use the new api in a local branch.
Just to mention I also did some basic memory leak stress tests with perl-libsass!

[mgreter]

Since nobody spoke up, I guess I can merge this, in about one hour!?
node-sass and perl-libsass are ready to move over to the new API.

[HamptonMakes]

NOOOO!!!!!

(just kidding)

[drewwells]

This is awesome! @mgreter have a beer on me @changetip

[changetip]

Hi mgreter, drewwells sent you a Bitcoin tip worth a beer (9,332 bits/$3.49), and I'm here to deliver it ➔ collect your tip at ChangeTip.com.

Learn more about ChangeTip

[am11]

@mgreter, unrelated question: does it throw segfault if we pass non-existing files path as input_path in sass_make_file_context? In case input file doesn't exist && source_string is empty as well, is it possible to a get proper error message: { file: input_file, message: "file not found" }?

[mgreter]

No, of course it will not segfault under that condition. And it should do exactly the same thing as with normal imports if you pass it only a file path. It will not try to load it if you also pass some content with it, then it is IMO only used for source-map path generating.

[am11]

I mean with the normal input_path, does it generate graceful errors? Because with node-sass it is terminates the interactive console if we pass {file: /path/to/non-existing.scss}.

For example with my node-sass master branch (with doesn't have importer code), this code:

require("./").render({
  file: "C:/temp/alpha/index2.scss",
  success: function (css,map) { console.log("css: " + css); console.log("map: " + map) },
  error: function (err) { console.log("err: " + JSON.stringify(err)) },
})

terminates the node console, because index2.scss doesn't exists on this path (as if segfault occurred).

[mgreter]

Hmm, seems like you found the first bug in the new API since I merged it.
Too strange it returned the correct error when I tried it with perl-libsass!?
Hotfix is here: https://github.com/mgreter/libsass/tree/hotfix/skip-compile-on-error
Apology for the inconvenience!

[am11]

Impressive! Thanks for the quick fix!! 😄 👍

I will try it later, right now I am working on a different computer.

[am11]

@mgreter, with node-sass' render function, we target sass_data_context (ctor takes string). If we pass both file and data, it picks file. As you mentioned, it should give precedence to data, in presence of both. I think this is a small glitch. :)

---

However, this is still debatable; whether or not it should prioritize data over file in case of sass_file_context (as its ctor takes input_path). From one view-point, data would have no meanings in case of sass_file_context.

In my opinion, we should just have one context, which may tolerate either, or and both: file and data. The following two ctors can be made available:

• sass_context (char* file, char* data); // data takes precedence and file is used for sourcemaps
• sass_context (char* input, enum sass_context_input_method input_method);

Where enum sass_context_input_method could be:

enum sass_context_input_method 
{ 
  FILE,
  DATA,
};

Or maybe, for sake of brevity; just use a c99 bool: sass_context (char* input, bool is_file). :)

Thoughts?

[mgreter]

Hmm, in case of sass_file_context there is no data or source_string, that is only available for sass_data_context, so I wonder if you really created the correct context!? I kept it apart as in the original implementation there was also a folder context declared (but never implemented).

[am11]

My bad: actually it is our script which is doing things. We have three methods: renderSync, renderFileSync and renderFile, which makes to file and data contexts. We will be making a lot of breaking changes for node.js v2.

Given the current state of matters, having two separate classes doesn't make much sense, since the new API is now streamlined. I think file vs. data can be unified and more configuration options can be introduced later. :)