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 panics from multiple borrows of Map. #1077

Merged
merged 1 commit into from Jan 19, 2021

Conversation

joshwd36
Copy link
Contributor

This Pull Request fixes/closes #1058.

It changes the following:

  • Move to an index-based approach to iterating during for-each.

This approach still has issues, as mentioned in #1058 (comment)

@codecov
Copy link

codecov bot commented Jan 17, 2021

Codecov Report

Merging #1077 (49a172c) into master (5a5061c) will decrease coverage by 0.01%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
- Coverage   58.70%   58.68%   -0.02%     
==========================================
  Files         176      176              
  Lines       12391    12405      +14     
==========================================
+ Hits         7274     7280       +6     
- Misses       5117     5125       +8     
Impacted Files Coverage Δ
boa/src/builtins/map/mod.rs 71.08% <70.00%> (-1.29%) ⬇️
boa/src/builtins/map/ordered_map.rs 57.57% <0.00%> (-6.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a5061c...49a172c. Read the comment docs.

@Razican
Copy link
Member

Razican commented Jan 18, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 78,497 78,497 0
Passed 24,805 24,813 +8
Ignored 15,587 15,587 0
Failed 38,105 38,097 -8
Panics 26 20 -6
Conformance 31.60 31.61 +0.01%

@Razican Razican added builtins PRs and Issues related to builtins/intrinsics bug Something isn't working labels Jan 18, 2021
@Razican Razican added this to the v0.12.0 milestone Jan 18, 2021
Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

Seems like a good solution to the panics.
This is a pretty weird part of the spec, certain callbacks can create infinite-loops:

function callingCallback(value, key, map) {
  console.log(key)
  map.delete(key)
  map.set(key, value)
}

Not sure how to implement this, might want to look at other engines' implementations.

Copy link
Contributor

@tofpie tofpie left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Borrow panics in Map
4 participants