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 output emitter and implement missing output styles #910

Merged
merged 2 commits into from
Mar 2, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Mar 1, 2015

This superseeds #899

Related to sass/sass-spec#270 and sass/sassc#97

  • Heavily refactored output code
  • Should make source maps more reliable
  • Output styles are mostly done in emitter.cpp
  • Refactored Parser (added new String type)
  • Interpolation should now work as with ruby sass
  • Commented out obsolete code (missing spec tests?)

Fixes

@mgreter mgreter force-pushed the feature/output-emitter branch 3 times, most recently from d9992d0 to d4a84d5 Compare March 1, 2015 12:44
@mgreter
Copy link
Contributor Author

mgreter commented Mar 1, 2015

@xzyfer IMHO this is ready to be merged! Anything speaking against it from your side? What do you think about the changes in the spec-test? Really hope everything is still in order 😅

@xzyfer
Copy link
Contributor

xzyfer commented Mar 1, 2015

The only think that jumped out at me is sass/sass-spec#270 (comment). Otherwise 👍

@am11
Copy link
Contributor

am11 commented Mar 2, 2015

👍

@am11
Copy link
Contributor

am11 commented Mar 2, 2015

AppVeyor builds are failing for VC jobs: https://ci.appveyor.com/project/sass/libsass/build/1.0.200.

Adding output_nested and output_compressed in win/libsass.vcxproj and applying the following patch would fix it:

diff --git a/inspect.cpp b/inspect.cpp
index d58dd3c..f3998b1 100644
--- a/inspect.cpp
+++ b/inspect.cpp
@@ -6,6 +6,7 @@
 #include <string>
 #include <iostream>
 #include <iomanip>
+#include <stdint.h>

@mgreter
Copy link
Contributor Author

mgreter commented Mar 2, 2015

@am11 I've added the additional header you mentioned in your comment above, but output_nested and output_compressed have been removed actually (or I did not understand your comment)!

@am11
Copy link
Contributor

am11 commented Mar 2, 2015

@mgreter, yes actually the error was about files not found (because they were referenced in vcxproj but were removed from repo). 😃

It is now complaining that we need #include<stdint.h> in util.cpp.
https://ci.appveyor.com/project/sass/libsass/build/1.0.207/job/bbhkel9xigunkppk#L950

Would it make sense to add headers in .hpps, so when the header is used by multiple .cpp, it shares forward declarations for those lang features?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.94%) to 80.67% when pulling 7877cfa on mgreter:feature/output-emitter into f642fad on sass:master.

@am11
Copy link
Contributor

am11 commented Mar 2, 2015

@mgreter
Copy link
Contributor Author

mgreter commented Mar 2, 2015

Thanks for the help with the appveyor ci build! 🚀

@am11
Copy link
Contributor

am11 commented Mar 2, 2015

@mgreter, there are around 266 warnings in VC job on AppVeyor. Some warnings are ignorable (POSIX style vs. STD-C++ style like _stdrup).
But others may be useful. Like there are 100+ warnings with code C4099: https://msdn.microsoft.com/en-us/library/695x5bes.aspx.

'Sass::Context' : type name first seen using 'struct' now seen using 'class'

(and vice versa)

For instance, in bind.hpp you have struct Context, while elsewhere it is used as class. Would it be a good idea to visit those warnings, it might improve some code patterns. :)

@mgreter
Copy link
Contributor Author

mgreter commented Mar 2, 2015

Maybe someone that actually uses MSVC could create a patch to solve these warnings? Beside a struct is principally just a class with all variables defined as public ... but wouldn't hurt to get rid of these. We also do have a copy_c_str function which could act as a replacement for strdup.

@mgreter
Copy link
Contributor Author

mgreter commented Mar 2, 2015

Since nobody has any objections against it, I'm going to hit "merge" here in a few minutes!

@am11
Copy link
Contributor

am11 commented Mar 2, 2015

Sure. I will try to take a stab at those soon. 😃

@xzyfer
Copy link
Contributor

xzyfer commented Mar 3, 2015

Amazing work @mgreter !!

@mgreter mgreter self-assigned this Mar 10, 2015
@mgreter mgreter deleted the feature/output-emitter branch April 6, 2015 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants