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

Fix: Map/Set polyfill should store -0 in keys as +0 #2905

Closed
wants to merge 1 commit into from

Conversation

@teppeis teppeis changed the title Fix: Map/Set should store -0 in keys as +0 Fix: Map/Set polyfill should store -0 in keys as +0 Apr 24, 2018
@brad4d
Copy link
Contributor

brad4d commented Apr 25, 2018

@teppeis are you sure this change is really needed?
I mean, did you actually see a case in running code where our polyfill for Map or Set treated +0 and -0 differently?

@shicks what do you think?

@teppeis
Copy link
Contributor Author

teppeis commented Apr 26, 2018

@brad4d
In actual code, -0 is rarely used.
However, I think that polyfill should conform to the specification as much as possible if it doesn't have performance or other issues obviously.

@brad4d
Copy link
Contributor

brad4d commented Apr 26, 2018

@teppeis what I'm saying is I suspect our code actually handles -0 correctly already even though that isn't visible in the code.
Can you show otherwise?

@teppeis
Copy link
Contributor Author

teppeis commented Apr 27, 2018

@brad4d Current polyfill handles -0 correctly to compare with stored keys (Map#get() or #has() work well), but the stored keys are not converted to +0 (Map#keys() or #forEach() don't work).
Following tests are failed.

const map = new Map().set(-0, 'foo');
assertEquals(Infinity, 1 / map.keys().next().value);

const set = new Set([-0]);
assertEquals(Infinity, 1 / set.keys().next().value);

@brad4d brad4d self-assigned this Apr 27, 2018
@brad4d
Copy link
Contributor

brad4d commented Apr 27, 2018

imported and sent for internal review

@brad4d
Copy link
Contributor

brad4d commented May 4, 2018

added a few more test cases
starting internal submission

@teppeis teppeis deleted the map-set-normalize-zero branch May 5, 2018 02:05
@teppeis
Copy link
Contributor Author

teppeis commented May 5, 2018

@brad4d @lauraharker thanks!

Yannic pushed a commit to Yannic/com_google_closure_compiler that referenced this pull request Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants