Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issue with invalid duplicate keys #2118

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Jul 3, 2016

Keys 1 and 1px were considered duplicates if they end
up in the same bucket. In sass these are indeed equal but
that does not seem to apply when used as hash keys.

@xzyfer feel free to refactor as you see fit.

@xzyfer xzyfer added this to the 3.3.7 milestone Jul 4, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Jul 4, 2016

Thanks @mgreter. Looks good. I'll write a test and merge soon.

Keys `1` and `1px` were considered duplicates if they end
up in the same bucket. In sass these are indeed equal but
that does not seem to apply when used as hash keys.
@mgreter mgreter force-pushed the bugfix/invalid-duplicate-map-key branch from 6f29dd8 to 4682c70 Compare August 3, 2016 15:12
@mgreter
Copy link
Contributor Author

mgreter commented Aug 3, 2016

@xzyfer this is a compiler edge case and only happens if the hash table implementation puts both keys in the same bucket for some reason (it should not). This is not the case in any of libsass' CI envs. I only saw it with some compilers and perl cpan "CI" (see links above).

@mgreter mgreter self-assigned this Aug 3, 2016
@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage increased (+0.005%) to 79.101% when pulling 4682c70 on mgreter:bugfix/invalid-duplicate-map-key into 55c0858 on sass:master.

@mgreter mgreter merged commit 5d844fd into sass:master Aug 3, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Aug 3, 2016

👍

@xzyfer xzyfer modified the milestones: 3.3.7, 3.4 Oct 20, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants