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

Specification compliant ToString (to_string) #425

Merged
merged 1 commit into from
Jun 1, 2020
Merged

Conversation

HalidOdat
Copy link
Member

This PR makes ToString (to_string) spec compliant.

What was wrong with the previous implementation?

The previous implementation used to ToString trait which was automatically implemented for Display, the problem is that we use different logic for displaying and converting to String type, for example, converting a string value to a String type with displays to_string is "some string value" (don't get the quotes, because the is a bug #373) but with the spec compliant function it's some string value without the quotes.
Additionally the spec compliant version should call to_primitive on objects and if it was to throw an exception it would have to propagate it.

This PR should unblock #373 .

Changes:

  • Moved Value display logic to builtins/value/display.rs
  • Added to_string to interpreter
  • Change most use of displays to_string => interpreters to_string

@HalidOdat HalidOdat added bug Something isn't working enhancement New feature or request technical debt labels May 30, 2020
@HalidOdat HalidOdat added this to the v0.9.0 milestone May 30, 2020
@github-actions
Copy link

Benchmark for 4b86b35

Click to view benchmark
Test PR Benchmark Master Benchmark %
Create Realm 448.1±33.79µs 445.9±34.56µs 100%
Expression (Lexer) 2.2±0.15µs 2.2±0.15µs 100%
Expression (Parser) 5.1±0.37µs 5.0±0.39µs 102%
Fibonacci (Execution) 2.8±0.18ms 2.9±0.18ms 98%
For loop (Execution) 463.9±29.04µs 483.3±42.29µs 96%
For loop (Lexer) 5.6±0.37µs 5.8±0.47µs 96%
For loop (Parser) 14.8±1.31µs 13.8±0.72µs 107%
Hello World (Lexer) 1082.6±99.57ns 1069.4±86.52ns 101%
Hello World (Parser) 2.4±0.18µs 2.4±0.20µs 99%
Symbols (Execution) 474.7±35.97µs 472.1±26.63µs 101%
undefined undefined 100%

Copy link
Member

@Razican Razican left a 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 comments in places I thought we could improve readability, mostly :)

boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/string/mod.rs Show resolved Hide resolved
boa/src/builtins/string/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/display.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/display.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/display.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/mod.rs Outdated Show resolved Hide resolved
boa/src/exec/mod.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jun 1, 2020

Benchmark for 8be4ad9

Click to view benchmark
Test PR Benchmark Master Benchmark %
Create Realm 385.8±22.33µs 394.5±24.61µs 98%
Expression (Lexer) 1874.8±124.62ns 1855.3±101.68ns 101%
Expression (Parser) 4.6±0.29µs 4.8±0.25µs 97%
Fibonacci (Execution) 2.6±0.12ms 2.6±0.13ms 100%
For loop (Execution) 417.5±29.20µs 453.0±22.94µs 90.99999999999999%
For loop (Lexer) 4.9±0.37µs 5.1±0.35µs 96%
For loop (Parser) 12.6±0.74µs 12.5±0.98µs 100%
Hello World (Lexer) 896.5±57.41ns 900.0±61.90ns 100%
Hello World (Parser) 2.1±0.11µs 2.1±0.14µs 101%
Symbols (Execution) 426.3±27.67µs 434.0±33.07µs 98%
undefined undefined 100%

 - Moved value display logic to value/display.rs
@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #425 into master will decrease coverage by 0.08%.
The diff coverage is 56.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
- Coverage   66.72%   66.64%   -0.09%     
==========================================
  Files         123      124       +1     
  Lines        8809     8852      +43     
==========================================
+ Hits         5878     5899      +21     
- Misses       2931     2953      +22     
Impacted Files Coverage Δ
boa/src/builtins/object/mod.rs 27.16% <0.00%> (ø)
boa/src/builtins/value/conversions.rs 44.59% <ø> (-1.46%) ⬇️
boa/src/builtins/value/mod.rs 58.60% <ø> (-2.22%) ⬇️
boa/src/exec/expression/mod.rs 71.42% <0.00%> (ø)
boa/src/builtins/console/mod.rs 27.31% <9.09%> (-3.68%) ⬇️
boa/src/builtins/string/mod.rs 36.90% <36.11%> (+0.14%) ⬆️
boa/src/builtins/bigint/mod.rs 52.63% <50.00%> (-1.92%) ⬇️
boa/src/builtins/number/mod.rs 71.35% <57.89%> (-1.23%) ⬇️
boa/src/exec/mod.rs 54.51% <70.58%> (+1.20%) ⬆️
boa/src/builtins/array/mod.rs 80.94% <75.00%> (+0.35%) ⬆️
... and 7 more

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 073b8ce...99c208e. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jun 1, 2020

Benchmark for f92de8e

Click to view benchmark
Test PR Benchmark Master Benchmark %
Create Realm 469.6±17.02µs 459.1±13.94µs 102%
Expression (Lexer) 2.2±0.04µs 2.2±0.03µs 101%
Expression (Parser) 5.3±0.21µs 5.4±0.22µs 99%
Fibonacci (Execution) 2.8±0.06ms 2.8±0.05ms 100%
For loop (Execution) 480.6±11.75µs 470.4±7.70µs 102%
For loop (Lexer) 5.7±0.16µs 5.8±0.12µs 99%
For loop (Parser) 14.3±0.47µs 14.5±1.16µs 99%
Hello World (Lexer) 1038.0±20.58ns 1048.7±19.74ns 99%
Hello World (Parser) 2.4±0.06µs 2.4±0.23µs 99%
Symbols (Execution) 487.4±5.82µs 483.9±10.21µs 101%
undefined undefined 100%

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This is good to go! :)

@Razican Razican merged commit 5e71718 into master Jun 1, 2020
@Razican Razican deleted the fix/to-string branch June 1, 2020 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants