-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
[Merged by Bors] - Implement WeakMap
#2597
Conversation
Test262 conformance changes
Fixed tests (218):
|
Codecov Report
@@ Coverage Diff @@
## main #2597 +/- ##
==========================================
- Coverage 48.84% 48.73% -0.11%
==========================================
Files 394 395 +1
Lines 39092 39199 +107
==========================================
+ Hits 19093 19104 +11
- Misses 19999 20095 +96
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! Just had a small nitpick which doesn't block merging this.
// 5. For each Record { [[Key]], [[Value]] } p of entries, do | ||
// a. If p.[[Key]] is not empty and SameValue(p.[[Key]], key) is true, return p.[[Value]]. | ||
// 6. Return undefined. | ||
if let Some(value) = m.get(key.inner()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use unwrap_or_default
instead.
I do want to take a proper look at this when I get some time, looks good so far though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bors r+ |
Pull request successfully merged into main. Build succeeded: |
This Pull Request changes the following:
WeakMap
buildin object.