-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Implement Set builtin object #1111
Conversation
Test262 conformance changes:
|
Codecov Report
@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
+ Coverage 58.53% 58.80% +0.27%
==========================================
Files 176 179 +3
Lines 12547 12821 +274
==========================================
+ Hits 7344 7540 +196
- Misses 5203 5281 +78
Continue to review full report at Codecov.
|
Benchmark for df00d23Click to view benchmark
|
Benchmark for 5f30569Click to view benchmark
|
Benchmark for d18aa53Click to view benchmark
|
Benchmark for c12c36aClick to view benchmark
|
Benchmark for 75806b3Click to view benchmark
|
Benchmark for 93823cbClick to view benchmark
|
Benchmark for 209e32aClick to view benchmark
|
Benchmark for 5d4c262Click to view benchmark
|
- Fix Map.length - Fix msg for invoking Map without new - Simplify calculation of map size when displaying Maps
- Introduce IteratorClose - Fix a few errors in Set covered by test262 - Fix switch of PropertyDescriptor kind - Fix deletion of accessor properties (UnaryOp change) - Fix symbol equality
Co-Authored-By: HalidOdat <[email protected]>
Benchmark for 7af5f9eClick to view benchmark
|
Benchmark for 3c6d330Click to view benchmark
|
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.
Looks very good!! I added some suggestions, but it's good to go from my side :) congrats!
Also make some minor changes to fulfill clippy lints
Benchmark for 77451b1Click to view benchmark
|
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.
Looks good to me! Just some minor documentation things.
The build failure is due to changes in |
Benchmark for f0a4b60Click to view benchmark
|
Speed up access to the prototype in the constructors Co-Authored-By: HalidOdat <[email protected]>
Benchmark for fb495bdClick to view benchmark
|
The MacOS CI "failure" was just a problem on GHs side, tests have been working so far, no reason to expect them to fail now. |
Co-authored-by: HalidOdat <[email protected]>
This Pull Request fixes/closes #400.
Besides implementing
Set
this PR also changes the following:Value::hash
so that1i32 == 1f64
Map.prototype.length
(was 1, should be 0 because the argument is optional)OrderedMap
rather than going throught the propertyValue::Symbol
The change to
Value::hash
was a bandaid fix to the fact that incrementing/decrementing integers with++
/--
results in a float, that should still be addressed, but I think this change would still be needed.ps: I'll leave the remaining tests failing, currently failing tests [19] can be grouped by:
WeakSet
to be implemented [14]Set.prototype.forEach
when elements are removed (tracked in Deleting elements from a map during forEach results in invalid iteration #1092 forMap
, solution will likely be easily translated toSet
) [2]this
to beundefined
in strict mode [1]$262
to be defined and use$262.createRealm()
[2]tests that expect(tested locally, they pass now that Implement Reflect built-in object #1033 has been merged)Reflect
to be implemented, which should pass once Implement Reflect built-in object #1033 is merged [2]