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

Refactor C++ context to expose call stack #1626

Merged
merged 7 commits into from
Nov 1, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Oct 26, 2015

This is a big refactor of the C++ context handling side. Basically we now directly use the C structs instead of converting them back and forth. Also made it more similar to the C-API by adding Data_Context and File_Context. We now also take care of initializing the options accordingly on the C++ side, so I was able to remove some stuff from the C-API.

I tried to streamline the import handling by adding specific classes for all different but similar representations of import, resources, etc. I think this will make the internas much clearer once it is used consistently throughout the code (I already got rid of Sass_Queues struct).

// requested import
class Importer {
  public:
    // requested import path
    std::string imp_path;
    // parent context path
    std::string ctx_path;
    // base derived from context path
    // this really just acts as a cache
    std::string base_path;
};

// a resolved include (final import)
class Include : public Importer {
  public:
    // resolved absolute path
    std::string abs_path;
};

// a loaded resource
class Resources {
  public:
    // the file contents
    char* contents;
    // conected sourcemap
    char* srcmap;
};

// parsed stylesheet from loaded resource
class StyleSheet : public Resources {
  public:
    // parsed root block
    Block* root;
};

Fixes #1638

@mgreter mgreter added this to the 3.3.1 milestone Oct 26, 2015
@mgreter mgreter self-assigned this Oct 26, 2015
@mgreter
Copy link
Contributor Author

mgreter commented Oct 26, 2015

This is pretty much ready, but could use some more testing. My poor memory leak tests seem to indicate that everything is freed as needed. There is room for more improvements though.

@mgreter
Copy link
Contributor Author

mgreter commented Oct 26, 2015

Just to repeat how to access the import stack during custom functions now:

union Sass_Value* call_sass_function(const union Sass_Value* s_args, Sass_Function_Entry cb, struct Sass_Compiler* compiler)
{
  struct Sass_Import* import = sass_compiler_get_last_import(compiler);
  const char* prev_abs_path = sass_import_get_abs_path(import);
  const char* prev_imp_path = sass_import_get_imp_path(import);
  ...
}

@mgreter
Copy link
Contributor Author

mgreter commented Oct 26, 2015

//CC @xzyfer @chriseppstein to fix eyeglass import once issue

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2015

@mgreter I pulled out a bunch of the code that wasn't relevant larger refactor into #1662, #1663, #1664, #1665, #1666

This also appears to include the rounding refactor from #1621.

Could you please rebase off master, and also remove those sass rounding commits?
Then I'll dig into this refactor. The scope now look small enough that this could land in 3.3.2.

@mgreter
Copy link
Contributor Author

mgreter commented Oct 28, 2015

Will do, actually the main commit is mgreter@1a68364 which is still rather big.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2015

I'll work through it over the next couple days. Nothing has jumped out at me yet. This is probably one of the last things before 3.3.2. Could you also update the markdown docs as well?

@mgreter
Copy link
Contributor Author

mgreter commented Oct 28, 2015

There is nothing to update in docs for this PR??
BTW. the wiki should be disabled now I guess ...

@mgreter
Copy link
Contributor Author

mgreter commented Oct 28, 2015

Disabled the wiki, since info is outdated and master is now in libsass repo ... maybe we could use git 2.6 subtree feature to get wiki visibility on github back ... still think submoduling gh-pages would also work.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2015

Sorry #1626 (comment) made me think there was

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2015

Can you also rebase out the rounding commits? They don't belong here.

@mgreter
Copy link
Contributor Author

mgreter commented Oct 29, 2015

Rebased, but will probably result in merge conflicts with the other "rounding" PR ...

@xzyfer
Copy link
Contributor

xzyfer commented Oct 29, 2015

That's ok. The rounding PR wont land in 3.3.2 because updating sass-spec to a new Ruby Sass breaks too many specs. (mostly on error messages / deprecation warnings)

@xzyfer
Copy link
Contributor

xzyfer commented Oct 29, 2015

LGTM. I'm not sold on the names of the new classes, purely because without the comments their purpose is unclear.

I would like to see #1626 (comment) added to documentation somewhere as part of this PR. Then feel free to 🚢

@mgreter
Copy link
Contributor Author

mgreter commented Nov 1, 2015

@xzyfer IMO rouding PR is quite important since we currently fail the spec tests when compiled for 32bit.

mgreter added a commit that referenced this pull request Nov 1, 2015
Refactor C++ context to expose call stack
@mgreter mgreter merged commit f89e8d7 into sass:master Nov 1, 2015
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.

Custom Importer caching bug?
2 participants