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

Refactoring and base work for 3.3 #1249

Merged
merged 10 commits into from Jul 13, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented May 30, 2015

Pushing my latest refactoring work to public since it is currently at a working state that solves all open @at-root issues, plus a few other open issues. PR is still too dirty too actually commit the work to master branch. It also introduced a minor whitespace (linefeed preserving) regressions (from issue 1208), since the fix we added for it doesn't seem to be 100% correct. I will try to tackle the linefeed preserving again at a later stage (the logic for this seems to be clear so far).

The following issues should pass with this WIP branch:

This branch also includes quite a few performance optimizations.
It brought my sass-spec test suite runtime from 27s down to 7s!

TODO

@mgreter mgreter added this to the 3.3 milestone May 30, 2015
@mgreter mgreter self-assigned this May 30, 2015
@drewwells
Copy link
Contributor

Holy balls that's an amazing speed up! Great work!
On Fri, May 29, 2015 at 7:15 PM Marcel Greter notifications@github.com
wrote:

Pushing my latest refactoring work to public since it is currently at a
working state that solves all open @at-root issues, plus a few other open
issues. PR is still too dirty too actually commit the work to master
branch. It also introduced a minor whitespace (linefeed preserving)
regressions (from issue 1208), since the fix we added for it doesn't seem
to be 100% correct. I will try to tackle the linefeed preserving again at a
later stage (the logic for this seems to be clear so far).

The following issues should pass with this WIP branch:

This branch also includes quite a few performance optimizations.

It brought my sass-spec test suite runtime from 27s down to 7s!

You can view, comment on, or merge this pull request online at:

#1249
Commit Summary

  • Refactor contextualize into eval [WIP]
  • Cleaning up [WIP]
  • Make more sense of functions and environments [WIP]
  • Clean up some constructor arguments [WIP]
  • Fix some MSVC warnings (and mute some others)
  • Remove eval snapshot feature [WIP]
  • More clean ups [WIP]
  • Add performance improvements [WIP]
  • Fix windows build (leaks memory) [WIP]
  • Test alternative memory managing model [WIP]
  • Refactor scss parser [WIP]
  • Pass 941 and 948 (regression on 1208) [WIP]
  • Refactor supports/feature query parsing [WIP]

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1249.

@xzyfer
Copy link
Contributor

xzyfer commented May 30, 2015

Great work. I'll dig into this over the next couple days.

I noticed some changes to cssize I need to look at closely. Currently it's
a direct port of the ruby version and I would prefer we don't drift. (On my
phone, I haven't looked at the code yet)
On 29 May 2015 18:37, "Drew Wells" notifications@github.com wrote:

Holy balls that's an amazing speed up! Great work!
On Fri, May 29, 2015 at 7:15 PM Marcel Greter notifications@github.com
wrote:

Pushing my latest refactoring work to public since it is currently at a
working state that solves all open @at-root issues, plus a few other open
issues. PR is still too dirty too actually commit the work to master
branch. It also introduced a minor whitespace (linefeed preserving)
regressions (from issue 1208), since the fix we added for it doesn't seem
to be 100% correct. I will try to tackle the linefeed preserving again
at a
later stage (the logic for this seems to be clear so far).

The following issues should pass with this WIP branch:

This branch also includes quite a few performance optimizations.

It brought my sass-spec test suite runtime from 27s down to 7s!

You can view, comment on, or merge this pull request online at:

#1249
Commit Summary

  • Refactor contextualize into eval [WIP]
  • Cleaning up [WIP]
  • Make more sense of functions and environments [WIP]
  • Clean up some constructor arguments [WIP]
  • Fix some MSVC warnings (and mute some others)
  • Remove eval snapshot feature [WIP]
  • More clean ups [WIP]
  • Add performance improvements [WIP]
  • Fix windows build (leaks memory) [WIP]
  • Test alternative memory managing model [WIP]
  • Refactor scss parser [WIP]
  • Pass 941 and 948 (regression on 1208) [WIP]
  • Refactor supports/feature query parsing [WIP]

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#1249.


Reply to this email directly or view it on GitHub
#1249 (comment).

@xzyfer
Copy link
Contributor

xzyfer commented May 31, 2015

Looked at the cssize changes, they're fine.

@onedayitwillmake
Copy link

Really liking the changes in this branch, everything is really making a lot more sense as someone less familiar with the code base!

@xzyfer
Copy link
Contributor

xzyfer commented Jun 5, 2015

@mgreter if you can get this ready for review in a week I'm cool with
shipping 3.3 soon.
On 5 Jun 2015 00:10, "Mario Gonzalez" notifications@github.com wrote:

Really liking the changes in this branch, everything is really making a
lot more sense as someone less familiar with the code base!


Reply to this email directly or view it on GitHub
#1249 (comment).

@mgreter
Copy link
Contributor Author

mgreter commented Jun 5, 2015

@onedayitwillmake Thanks a lot for your feedback! I didn't actually do much work in the parser/eval before, so I had the get myself familiar with it first too. I see some more work in pseudo selectors, since they trigger specific "is_superselector" behavior. But we may postpone this after an initial release.

@xzyfer I'd say lets set a due date and I will see how much I can get in. I'm now at a stage where I think I have parent selectors covered quite well (took me about 3 days to get it right and to wrap my head around it). Unfortunately I will not have that much time over the next weekends.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.83%) to 78.82% when pulling 8f00b20 on mgreter:refactoring/parser-for-3.3 into 25a1d79 on sass:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.37%) to 78.27% when pulling 7411b6b on mgreter:refactoring/parser-for-3.3 into 25a1d79 on sass:master.

@mgreter mgreter force-pushed the refactoring/parser-for-3.3 branch from 7411b6b to a71cfad Compare June 12, 2015 16:55
@mgreter
Copy link
Contributor Author

mgreter commented Jun 12, 2015

Made some major break thoughs by removing the re-parsing step in ruleset expander. Basically re-implemented the complete selector parsing, expanding and evaluating part. Big performance improvements in the loop and number evaluation code parts and also a lot of smaller performance improvements from parser, expand and eval refactoring.

This branch should now pass the following open issues:

#941, #948, #1029, #1043, #1061, #1170, #1207, #1210, #1214, #1219, #1227, #1230, #1240, #1253, #1255, #1257, #1258, #1260, #1263, #1269, #1273, #1281, #1283, #1285, #1291, #1297

Now also passes these is_superselector tests:

  • has_is_superselector_of_subset_host
  • host_context_is_superselector_of_subset_host
  • host_is_superselector_of_subset_host
  • matches_is_superselector_of_subset_matches
  • nth_match_is_not_superselector_of_nth_last_match
  • nth_match_is_not_superselector_of_nth_match_with_different_arg

There is one minor white-space issue, that prevents

Example:

@mixin dummy {
  @keyframes {
    @content;
  }
}
@include dummy {
  foo, bar { baz: test; }
}

Ruby sass output (compressed!):

@keyframes{foo, bar{baz:test}}

Current libsass output (compressed!):

@keyframes{foo,bar{baz:test}}

IMHO libsass is correct here to remove the space after the comma separator.
@xzyfer if you agree here, I will commit the spec changes once the PR is merged!?

@mgreter mgreter force-pushed the refactoring/parser-for-3.3 branch 3 times, most recently from f53be75 to 133b20a Compare June 12, 2015 19:11
@mgreter
Copy link
Contributor Author

mgreter commented Jun 12, 2015

@xzyfer cleaned up the most obvious stuff but all in all I'm pretty much done for the first step. So this is pretty much open for review and comments now. I added a few crutches to solve a few edge cases for now. A complete solution for these edge cases will need some more work. These are namely:

  • wrapper argument for is_superselector evaluating
  • use of remove_parent_selectors in is_superselector
  • passing of is_root with block parsing function
  • few parsing functions are not coded very optimal

Other than that I feel pretty confortable with the final result!
Should be a solid base to work towards the next 3.3 release!

ToDo: need to check if parser errors out correctly for missing scope brackets and semicolons ...

@mgreter mgreter force-pushed the refactoring/parser-for-3.3 branch from 133b20a to 7ccfaca Compare June 13, 2015 15:55
@mgreter mgreter changed the title [WIP] Refactoring parser for 3.3 Refactoring parser for 3.3 Jun 14, 2015
@mgreter mgreter force-pushed the refactoring/parser-for-3.3 branch 2 times, most recently from a4fd8b0 to a262eed Compare June 15, 2015 20:54
@xzyfer
Copy link
Contributor

xzyfer commented Jul 13, 2015

I can confirm the final two todo items fixed in your latest commit.

Can you please rebase case Complex_Selector::REFERENCE: cerr << "{@} "; break; out of 9f5ef6d. I ran into it when doing a bisect today. I see you removed it in 3af051e anyway but lets remove it from the source.

@xzyfer
Copy link
Contributor

xzyfer commented Jul 13, 2015

It's midnight here. I'll rerun this branch over our code base in the morning. If there are no other regressions I'll ship it!

@xzyfer
Copy link
Contributor

xzyfer commented Jul 13, 2015

I've done some quick testing and I'm see regressions in newlines in selectors. A quick example

%foo {
  bar > bam &:before,
  bar > bam &:after {
    bar: baz;
  }
}

test {
  @extend %foo;
}

Result

bar > bam test:before,

bar > bam test:after {
  bar: baz; }

master

bar > bam test:before,
bar > bam test:after {
  bar: baz; }

Ruby Sass

bar > bam test:before, bar > bam test:after {
  bar: baz; }

Removing the & fixes the output.

%foo {
  bar > bam:before,
  bar > bam:after {
    bar: baz;
  }
}

test {
  @extend %foo;
}

Result

test bar > bam:before,
test bar > bam:after {
  bar: baz; }

master

test bar > bam:before,
test bar > bam:after {
  bar: baz; }

Ruby Sass

test bar > bam:before, test bar > bam:after {
  bar: baz; }

@xzyfer
Copy link
Contributor

xzyfer commented Jul 13, 2015

Also seeing new lines in selectors appearing after combinators

foo {
  > bar {
    > baz,
    > bam {
      bar: true;
    }
  }
}

Result

foo > bar > baz, foo >
bar > bam {
  bar: true; }

Master & Ruby Sass

foo > bar > baz,
foo > bar > bam {
  bar: true; }

@@ -899,6 +900,14 @@ namespace Sass {
Compound_Selector* head = c->head();
Complex_Selector* tail = c->tail();
Complex_Selector::Combinator comb = c->combinator();

if (c->has_line_feed()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This block appears to be the cause of the broken selector output reported in #1249 (comment) and #1249 (comment)

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 already got a fix for the first issue ... they don't seem related much ...

@mgreter
Copy link
Contributor Author

mgreter commented Jul 13, 2015

Another regression:

%foo1 {
  > bar > bam &:before,
  > bar > bam &:after {
    bar: baz;
  }
}

test1 {
  @extend %foo1;
}

Result before the fix:

> bar > bam &:before, > bar > bam &:after {
  bar: baz; }

Result after additional fix:

> bar > bam test1:before,
> bar > bam test1:after {
  bar: baz; }

IMO the additional line-feed makes sense and improves readability (and is more what I would expect from the given input). So I vote to leave it that way and call it a ruby sass bug!? IMHO once everything is more stable it should be possible to simplify the code even more. There are quite a few duplicated code points.

@xzyfer
Copy link
Contributor

xzyfer commented Jul 13, 2015

So I vote to leave it that way and call it a ruby sass bug

👍 I don't care much for matching Sass' whitespace semantics perfectly because they're too unpredictable. Like you say as long as the output is readable it's fine.

@xzyfer
Copy link
Contributor

xzyfer commented Jul 13, 2015

If CI goes green 🚢 it

🎉 🎆 🍕

@mgreter
Copy link
Contributor Author

mgreter commented Jul 13, 2015

Waiting for AppVeyor then ... 🚀 🎸

@am11
Copy link
Contributor

am11 commented Jul 14, 2015

Congrats ㊗️

mgreter added a commit to mgreter/sass-spec that referenced this pull request Jul 15, 2015
mgreter added a commit to mgreter/sass-spec that referenced this pull request Jul 15, 2015
@mgreter mgreter deleted the refactoring/parser-for-3.3 branch July 28, 2015 10:03
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.

None yet

10 participants