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

C-API: Add call stack queries (functions and mixins) #2251

Merged
merged 6 commits into from
Jan 5, 2017

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Dec 18, 2016

Same as #2239 but rebased and adjusted after memory refactoring

@mgreter
Copy link
Contributor Author

mgreter commented Dec 18, 2016

Fixes #1037 and #1950

Corresponding commits in perl-libsass:

Sample perl program

test.pl

use strict;
use warnings;

use CSS::Sass qw(get_callee_stack_size);
use CSS::Sass qw(get_callee_type);
use CSS::Sass qw(get_callee_name);
use CSS::Sass qw(get_callee_path);
use CSS::Sass qw(get_callee_line);
use CSS::Sass qw(get_callee_column);
use CSS::Sass qw(get_callee_var);
use CSS::Sass qw(set_callee_var);

my $sass = CSS::Sass->new(
  sass_functions => {
    'get-callee-stack-size()' => sub { get_callee_stack_size() },
    'get-callee-type($back : 0)' => sub { get_callee_type($_[0]) },
    'get-callee-name($back : 0)' => sub { get_callee_name($_[0]) },
    'get-callee-path($back : 0)' => sub { get_callee_path($_[0]) },
    'get-callee-line($back : 0)' => sub { get_callee_line($_[0]) },
    'get-callee-column($back : 0)' => sub { get_callee_column($_[0]) },
    'get-callee-var($name, $back : 0)' => sub { get_callee_var($_[0], $_[1]) },
    'set-callee-var($name, $val, $back : 0)' => sub { set_callee_var($_[0], $_[1], $_[2]) },
    'get-callee($back: 0)' => sub {
      return sprintf("(%d) %s at %s (%d/%d)",
        get_callee_type($_[0]),
        get_callee_name($_[0]),
        get_callee_path($_[0]),
        get_callee_line($_[0]),
        get_callee_column($_[0]),
      );
    }
  }
);

print $sass->compile_file("test.scss");

test.scss

@function baz($back: 0) {
  @return get-callee($back);
}

@function bar($back: 0) {
  @return baz($back);
}

@function foo($back: 0) {
  @return bar($back);
}

@mixin foo($back: 0) {
  @content;
}

A {
  $foo: "foo";
  @include foo {
    callee-5: foo(5);
    callee-4: foo(4);
    callee-3: foo(3);
    callee-2: foo(2);
    callee-1: foo(1);
    callee-0: foo(0);
    foo: get-callee-var("$foo");
    foo: set-callee-var("$foo", "new");
    foo: get-callee-var("$foo");
  }
}

result

A {
  callee-5: (0) foo at D:/.../perl-libsass/test.scss (19/12);
  callee-4: (0) @content at D:/.../perl-libsass/test.scss (14/3);
  callee-3: (1) foo at D:/.../perl-libsass/test.scss (21/15);
  callee-2: (1) bar at D:/.../perl-libsass/test.scss (10/11);
  callee-1: (1) baz at D:/.../perl-libsass/test.scss (6/11);
  callee-0: (2) get-callee at D:/.../perl-libsass/test.scss (2/11);
  foo: foo;
  foo: new; }

@mgreter mgreter changed the title Feature/c api fn call stack C-API: Add call stack queries (functions and mixins) Dec 18, 2016
@mgreter mgreter added the C API label Dec 18, 2016
@mgreter mgreter force-pushed the feature/c-api-fn-call-stack branch 2 times, most recently from b01e9ec to 1302cef Compare December 18, 2016 14:13
@mgreter mgreter self-assigned this Dec 19, 2016
@mgreter mgreter force-pushed the feature/c-api-fn-call-stack branch 11 times, most recently from cc3c24e to a883321 Compare December 28, 2016 05:05
Copy link
Collaborator

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

My C/C++ is horrible, just read through as best I could

@@ -87,8 +87,9 @@ namespace Sass {

{

// add cwd to include paths
include_paths.push_back(CWD);
// Sass 3.4: The current working directory will no longer be placed onto the Sass load path by default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it work wrapping in a complier directive for the SASS_LANG = 3.4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I was under the impression current master is going for 3.5. Side note: we could use a runtime test for 3.4 vs 3.5 behavior, but a compiler directive is not simple, since we have a string in that constant (string compare is not really supported by #if preprocessor directives).

Copy link
Contributor Author

@mgreter mgreter Dec 29, 2016

Choose a reason for hiding this comment

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

Just realized that this change was done in ruby sass 3.4, not 3.5, therefore I think we should already have that removed. So to speak this is a bug in libsass 3.4. Or I did not get your comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right that this is 3.5 now, but I was thinking for ease of backporting changes to 3.4 while they are still both being maintained. I guess it would need to be a plain definition of SASS_LANGUAGE_3_4

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 created a seperate commit for it, so it's better visible in this PR (for cherry-pick). IMO implementers are still free to add this on their own if the want to keep compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

3.4 is stable, so no disruptive changes will back ported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually a bugfix as ruby sass 3.4 removed this behavior and we did not yet ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, however it's a breaking change and 3.4 is stable. The ship has sailed on breaking changes in 3.4. Back ports are only happening for small Sass fixes.

@@ -113,7 +121,10 @@ namespace Sass {

namespace File {

std::vector<Include> resolve_includes(const std::string& root, const std::string& file);
static std::vector<std::string> defaultExtensions = { ".scss", ".sass", ".css" };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why include CSS files?

Copy link
Contributor Author

@mgreter mgreter Dec 28, 2016

Choose a reason for hiding this comment

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

Same behavor as before, also see #754

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, but isn't it going away by 4 though? Might be another one to conditional out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it should move into C-API to be configurable. This is just a small step in that direction.

char* ADDCALL sass_compiler_find_include (const char* file, struct Sass_Compiler* compiler)
{
// get the last import entry to get current base directory
// struct Sass_Options* options = sass_compiler_get_options(compiler);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, commented due to unused var compiler warning.

char* ADDCALL sass_compiler_find_file (const char* file, struct Sass_Compiler* compiler)
{
// get the last import entry to get current base directory
// struct Sass_Options* options = sass_compiler_get_options(compiler);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, commented due to unused var compiler warning.

@mgreter
Copy link
Contributor Author

mgreter commented Dec 28, 2016

Needs a rebase after #2267 is merged!

@mgreter
Copy link
Contributor Author

mgreter commented Dec 29, 2016

A few C-API remarks

The C-API has grown quite a bit and we now expose quite a few arrays. Implementation for them is often very different. Since the whole C-API is behind function calls and pointers now, we can use C++ behind those functions. Therefore we could just use std::vector instead of our own primitive implementations (as i.e. for include_paths, which is really a linked list in order to grow dynamically). But due to where we came from, most things on the C-API are still just structs (which are allocated via calloc and deleted via free). In order to support C++ containers, those structs need to be upgraded to real classes and allocated via new and freed via delete (otherwise we leak). So this task would basically boil down to design and implement a C++ API for LibSass (which would be a nice addon).

More CI tests (plugins)

I've added my libsass plugins to the CI process (Travis only so far). This allows most of the C-API to be tested via CI. I created a specific libsass-tests plugin to expose most C-API features as custom functions (work in progress). I was hoping to include a small sass-spec suite for each plugin, but unfortunately ran against sass/sass-spec#1025. So for now each plugin contains just one spec test that is run and compared via diff in libsass Travis CI.

For reference, these are the tested plugins:

*digest needs this PR to compile/work!

@mgreter mgreter force-pushed the feature/c-api-fn-call-stack branch 5 times, most recently from 9d070e8 to a2be21d Compare December 29, 2016 03:33
@xzyfer
Copy link
Contributor

xzyfer commented Jan 2, 2017

Hopefully this goes without saying but please don't start working on a C++ API before opening some RFCs to discuss the implementation and its goals.

@mgreter mgreter force-pushed the feature/c-api-fn-call-stack branch from a2be21d to 5f9c8fe Compare January 2, 2017 15:55
- Removes sass_option_get_plugin_path(opt)
- Removes sass_option_get_include_path(opt)

Removed path options are colon-separated lists of paths.
Most implementations will probably only want to use
the push functions for paths. We still support the old
way to set this option in order to help support SASS_PATH
environment variables (implementers can just pass it in).
Use the new query functions if you need them.

- Adds sass_option_get_include_path_size(opt)
- Adds sass_option_get_include_path(opt, i)
- Fixes a minor memory leak with plugins
- Adds sass_compiler_find_include(file, comp)
- Adds sass_compiler_find_file(file, comp)
- Adds sass_find_include(file, opt)
- Adds sass_find_file(file, opt)
Sass 3.4: The current working directory will no longer
be placed onto the Sass load path by default. If you
need the current working directory to be available,
set SASS_PATH=. in your shell's environment.
@typoworx-de
Copy link

How to use this in js using node-sass?

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.

4 participants