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

improve map conformance without losing perf #1445

Merged
merged 5 commits into from
Sep 11, 2021
Merged

Conversation

neeldug
Copy link
Contributor

@neeldug neeldug commented Jul 29, 2021

Improves conformance but avoids following spec directly for performance implications.

@neeldug
Copy link
Contributor Author

neeldug commented Jul 29, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 80,804 80,804 0
Passed 32,992 33,006 +14
Ignored 15,896 15,896 0
Failed 31,916 31,902 -14
Panics 0 0 0
Conformance 40.83% 40.85% +0.02%
Fixed tests (14):
test/built-ins/Map/prototype/get/returns-value-normalized-zero-key.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/get/returns-value-normalized-zero-key.js (previously Failed)
test/built-ins/Map/prototype/forEach/first-argument-is-not-callable.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/forEach/first-argument-is-not-callable.js (previously Failed)
test/built-ins/Map/prototype/set/replaces-a-value-normalizes-zero-key.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/set/replaces-a-value-normalizes-zero-key.js (previously Failed)
test/built-ins/Map/prototype/set/append-new-values-normalizes-zero-key.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/set/append-new-values-normalizes-zero-key.js (previously Failed)
test/built-ins/Map/prototype/clear/context-is-not-map-object.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/clear/context-is-not-map-object.js (previously Failed)
test/built-ins/Map/prototype/clear/context-is-set-object-throws.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/clear/context-is-set-object-throws.js (previously Failed)
test/built-ins/Map/prototype/clear/context-is-not-object.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/clear/context-is-not-object.js (previously Failed)

@Razican Razican added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request labels Aug 23, 2021
@Razican Razican added this to the v0.13.0 milestone Aug 23, 2021
@Razican
Copy link
Member

Razican commented Aug 23, 2021

Hello @neeldug ! thank you for the contribution and sorry for the delay in the review. It seems this requires a rebase, and it seems to be failing some clippy checks.

add clear & foreach & delete
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Some more changes and questions.

boa/src/builtins/map/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/map/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/map/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/map/mod.rs Outdated Show resolved Hide resolved
@neeldug
Copy link
Contributor Author

neeldug commented Sep 10, 2021

@jedel1043 Did check and it's unnecessary for delete, and for clear this should work, couldn't find a clean way to replace the reference with a new map so went into OrderedMap to create a method.

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Small nitpicks. We can merge it after this gets fixed :D

boa/src/builtins/map/ordered_map.rs Outdated Show resolved Hide resolved
boa/src/builtins/map/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Great work!

@jedel1043 jedel1043 merged commit 2f8c35d into boa-dev:master Sep 11, 2021
@neeldug neeldug deleted the map-conf branch September 11, 2021 01:32
neeldug added a commit to neeldug/boa that referenced this pull request Sep 15, 2021
Razican pushed a commit that referenced this pull request Sep 16, 2021
@raskad raskad mentioned this pull request Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants