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

Add generic support for destructuring assignment in @each #571

Merged
merged 2 commits into from
Oct 28, 2014

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Oct 27, 2014

This PR adds generic support for destructuring assignment in @each.

Fixes #492, #577. Specs added sass/sass-spec#54, sass/sass-spec#98, sass/sass-spec#99.

@xzyfer
Copy link
Contributor Author

xzyfer commented Oct 27, 2014

There's been a bit of activity tonight. I'm going to let this rest until tomorrow.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) when pulling 60a2107 on xzyfer:feat/each-maps-for-realz into 02f9058 on sass:master.

@am11
Copy link
Contributor

am11 commented Oct 27, 2014

Thanks for the fix. 👍

Can you please also make these changes (to stop msvcr from barfing: warning C4101: 'bc' : unreferenced local variable):

diff --git a/ast.hpp b/ast.hpp
index 59b9a4e..3bfac33 100644
--- a/ast.hpp
+++ b/ast.hpp
@@ -658,7 +658,7 @@ namespace Sass {
           if (!(*(elements()[i]) == *(l[i]))) return false;
         return true;
       }
-      catch (std::bad_cast& bc)
+      catch (std::bad_cast&)
       {
         return false;
       }
@@ -706,7 +706,7 @@ namespace Sass {
           if (!(*at(key) == *m.at(key))) return false;
         return true;
       }
-      catch (std::bad_cast& bc)
+      catch (std::bad_cast&)
       {
         return false;
       }
@@ -815,7 +815,7 @@ namespace Sass {
         Variable& e = dynamic_cast<Variable&>(rhs);
         return e && name() == e.name();
       }
-      catch (std::bad_cast& bc)
+      catch (std::bad_cast&)
       {
         return false;
       }
@@ -969,7 +969,7 @@ namespace Sass {
         e.normalize(find_convertible_unit());
         return unit() == e.unit() && value() == e.value();
       }
-      catch (std::bad_cast& bc)
+      catch (std::bad_cast&)
       {
         return false;
       }
@@ -1008,7 +1008,7 @@ namespace Sass {
         Color& c = (dynamic_cast<Color&>(rhs));
         return c && r() == c.r() && g() == c.g() && b() == c.b() && a() == c.a();
       }
-      catch (std::bad_cast& bc)
+      catch (std::bad_cast&)
       {
         return false;
       }
@@ -1044,7 +1044,7 @@ namespace Sass {
         Boolean& e = dynamic_cast<Boolean&>(rhs);
         return e && value() == e.value();
       }
-      catch (std::bad_cast& bc)
+      catch (std::bad_cast&)
       {
         return false;
       }
@@ -1120,7 +1120,7 @@ namespace Sass {
         String_Constant& e = dynamic_cast<String_Constant&>(rhs);
         return e && unquoted_ == e.unquoted_;
       }
-      catch (std::bad_cast& bc)
+      catch (std::bad_cast&)
       {
         return false;
       }

Also, it gives few other warnings: https://gist.github.com/am11/5324b8ffb44f0a599329. It might make sense to fix them, perhaps to save couple of milliseconds here and there? #519 is related.

@mgreter
Copy link
Contributor

mgreter commented Oct 27, 2014

Can we also get rid of the size_t hash_ = 0 initialization in ast.hpp? It breaks my travis-ci builds for perl-libsass (I'm compiling with -std=c++0x). Don't know where to initialize them properly though.

@am11
Copy link
Contributor

am11 commented Oct 27, 2014

Guys I have made the changes. Please review #572.

@QuLogic
Copy link
Contributor

QuLogic commented Oct 27, 2014

Can we also get rid of the size_t hash_ = 0 initialization in ast.hpp? It breaks my travis-ci builds for perl-libsass (I'm compiling with -std=c++0x). Don't know where to initialize them properly though.

Probably have to move it to the initialization list in the constructor (after the name and : but before the {).

@mirisuzanne
Copy link

@xzyfer This branch passes all my basic map tests. I ran this series of tests (with the final @each loop un-commented):

.test-each {
  -map-get: 'yep';
  -map-remove: 'yep';
  -map-merge: 'yep';
  -map-keys: 'yep';
  -map-values: 'yep';
  -map-has-key: 'yep';
  -map-compare: 'yep'; }

That seems to fix merge, compare, and loop. I'll also run it against larger frameworks when I get a chance. Cheers! :)

@xzyfer
Copy link
Contributor Author

xzyfer commented Oct 27, 2014

I'd prefer to limit the scope of this PR. Focused PRs make for better auditing and regression tracking.

@am11 thanks for opening that PR. Looks fine from a cursory glance. I'll take a closer look after work.

@mgreter @QuLogic I'll look at how I can move the hash_ initialisation. It's used for most Expressions not just lists, but is tied into Vectorized and Hashed. My feeling is that there will need to be a better interface to handle hash caching. Again I'd prefer to handle this in another PR since it's related to different feature.

@@ -162,7 +162,11 @@ namespace Sass {
void Inspect::operator()(Each* loop)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akhleung I'm a bit foggy on the purpose inspect.ccp, especially in regards to control structures like @each. Could you please shed some light?

Choose a reason for hiding this comment

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

@xzyfer The Inspect visitor is meant for rendering internal nodes back into a textual format (and the To_String visitor is defined using Inspect). In addition to rendering the final CSS output, it's also useful for debugging, in case you just need to print out internal nodes in your debug logs.

Control directives such as @each aren't present in the fully expanded tree, so they'll never be rendered in the final output. But the Inspect visitor still handles them in case you really need to print them out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Control directives such as @each aren't present in the fully expanded tree, so they'll never be rendered in the final output. But the Inspect visitor still handles them in case you really need to print them out.

Ah great this is the use case I wasn't aware of. Thanks for the clarification.

@xzyfer
Copy link
Contributor Author

xzyfer commented Oct 27, 2014

Thanks @ericam ! The map api should work as expected on the current master as of about 12hrs ago 🎉

Any edge cases you stumble across in @each would be great to test.

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2014

The tip by @QuLogic gave me the right directions, please see here for a IMO proper fix: mgreter@4fa3014

@xzyfer
Copy link
Contributor Author

xzyfer commented Oct 28, 2014

Looks sane to me! Can you put that in a PR?

@xzyfer
Copy link
Contributor Author

xzyfer commented Oct 28, 2014

This now passes all the specs. I'll wait for the above spec to be merged before merging this.

@xzyfer xzyfer force-pushed the feat/each-maps-for-realz branch from 9cac507 to 0a643df Compare October 28, 2014 10:59
@xzyfer xzyfer changed the title [WIP] Add generic support for destructuring assignment in @each Add generic support for destructuring assignment in @each Oct 28, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) when pulling 9cac507 on xzyfer:feat/each-maps-for-realz into 02f9058 on sass:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 0a643df on xzyfer:feat/each-maps-for-realz into 52ad379 on sass:master.

@xzyfer xzyfer force-pushed the feat/each-maps-for-realz branch from 0a643df to e8c549a Compare October 28, 2014 15:52
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) when pulling e8c549a on xzyfer:feat/each-maps-for-realz into 52ad379 on sass:master.

@HamptonMakes
Copy link
Member

If I'm reading the above thread correctly, seems like this is ready to be merged in. So, I'm doing it... like now. ;P

HamptonMakes added a commit that referenced this pull request Oct 28, 2014
Add generic support for destructuring assignment in @each
@HamptonMakes HamptonMakes merged commit f55b2d5 into sass:master Oct 28, 2014
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.

@each with maps
8 participants