Error when declarations are printed without ruleset #2062
Conversation
xzyfer
changed the title
|
|
||
| Statement* CheckNesting::operator()(Declaration* d) | ||
| { | ||
| if (block_stack.size() < 2) { |
There was a problem hiding this comment.
This check still needs work and the test case will need to be updated.
|
|
||
| Statement* CheckNesting::operator()(Block* b) | ||
| { | ||
| Block* bb = SASS_MEMORY_NEW(ctx.mem, Block, b->pstate(), b->length(), b->is_root()); |
There was a problem hiding this comment.
Is it really necessary to make another copy? I figure this should not be needed since we do not really alter anything here? Creating too many clones is the main reason why libsass is slower than it could be (I already removed some cloning in eval some time ago, which lead to the big performance gain). The other reason we are slower than needed is that we visit too many visitors, but I guess there is not much too avoid that and it should not have a big impact ...
There was a problem hiding this comment.
No we don't need the copy. I slimmed this down a lot, fixed the check, and beefed out the spec. This visitor should never mutate the nodes.
There was a problem hiding this comment.
IMHO if it should never mutate than the append_block is also superflous. Doesn't it always replace the children with itself anyway under that condition? Otherwise good work!
Probably something like that should do it:
Statement* CheckNesting::operator()(Block* b)
{
for (auto child : b->elements()) {
child->perform(this);
}
}There was a problem hiding this comment.
Yep I've also removed that locally. Just pushed my latest. Tracking down an error mismatch atm.
|
Other than my comments, LGTM! |
xzyfer
force-pushed
the
fix/issue-1732
branch
2 times, most recently
from
a396a3f to
2e7810d
Compare
|
Beefed out the specs in sass/sass-spec#857 |
This is introduces a vastly reduced `CheckNesting` visitor from Ruby Sass as discussed in sass#2061. Doing this properly will require some small changes to the parser, and probably eval, which currently try to do some nesting checking of their own. This is just first step to properly introducing the proper nesting checking. Closes sass#2061 Fixes sass#1732
xzyfer
changed the title
The check nesting visitor is responsible for AST validation. This was left over from sass#2062. Also fixed a bug that caused the function child node validation to be skipped.
The check nesting visitor is responsible for AST validation. This was left over from sass#2062.
The check nesting visitor is responsible for AST validation. This was left over from #2062. Also fixed a bug that caused the function child node validation to be skipped.
The check nesting visitor is responsible for AST validation. This was left over from #2062.
This is introduces a vastly reduced
CheckNestingvisitor from RubySass as discussed in #2061. Doing this properly will require some
small changes to the parser, and probably eval, which currently try
to do some nesting checking of their own.
This is just first step to properly introducing the proper nesting
checking.
Spec sass/sass-spec#857
Closes #2061
Fixes #1732