-
Notifications
You must be signed in to change notification settings - Fork 464
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/directive parsing #2001
Conversation
e46ab12
to
fd0f1c6
Compare
fd0f1c6
to
c4f4ef3
Compare
c4f4ef3
to
83d401b
Compare
If you're going to make this massive of a change I suggest doing your best to move more inline with Ruby Sass implementation like with cssize. The closer our internal data flow and logic is to Ruby the easier it's going to be keep features in sync, and prevent weird edge cases. |
If you check the commits this is exactly what is happening here (see prelexer)! |
I wasn't insinuating it wasn't what you were doing. I just wanted to make sure we were on the same page. |
Just assumed, because the main reason why this change is that massive is because it is following ruby sass closely in the first place 😉 |
Just to be clear, I agree to follow ruby sass closely when it comes to the parser and the internal ast in general. But we can (and should) do better in libsass with manipulation and processing. I guess ruby sass choose to do quite a few operations (I'm specially refering to extend) in a monolithic approach. This makes sense in a language with dynamic function dispatching, since adding to many method calls in tight loops (code hotspots) can lead to bad performance. We don't have this constraint in c++ and should embrace the possibility of micro functions for selector manipulations. Another reason for that is that writing big monolithic functions in C++ (or any strict types language for that matter) tends to get tiresome, specially when a lot of loops are involved. But so far extend works and I don't want to touch any of that directly ported |
I agree. I tend to avoid the extend code this exact reason. |
You'll probably have to rebase your sass-spec branch of master. |
Spec test sass/sass-spec#764 (excerpt from #1900, fixes #1233)