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

Removed duplicated code in vm.run using macros. #1044

Merged
merged 1 commit into from
Mar 7, 2021

Conversation

stephanemagnenat
Copy link
Contributor

@stephanemagnenat stephanemagnenat commented Jan 6, 2021

This Pull Request reduces code duplication in VM execution, this refactoring let me find issue #1041.

It changes the following:

  • It factors out the push of the result to the stack.
  • It adds a macro for binary operations.

I did not use the macro equality, in order to keep the balance with the inequality code which, due to the ! before the into, would still need the macro. When using the binop macro, one might want to put the call in a single line instead of using { ... }.

I do not know whether upstream wants this change, but I submit it just in case. I do not know if the vm feature is enabled in CI.

This PR also fixes #1041.

@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #1044 (4e69075) into master (9e0cab5) will increase coverage by 0.35%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
+ Coverage   58.53%   58.88%   +0.35%     
==========================================
  Files         176      172       -4     
  Lines       12547    11709     -838     
==========================================
- Hits         7344     6895     -449     
+ Misses       5203     4814     -389     
Impacted Files Coverage Δ
boa/src/vm/mod.rs 0.00% <0.00%> (ø)
boa/src/syntax/lexer/string.rs 38.09% <0.00%> (-26.32%) ⬇️
boa/src/builtins/boolean/mod.rs 30.00% <0.00%> (-13.91%) ⬇️
boa/src/builtins/error/uri.rs 65.00% <0.00%> (-5.00%) ⬇️
boa/src/builtins/error/eval.rs 65.00% <0.00%> (-5.00%) ⬇️
boa/src/builtins/error/type.rs 65.00% <0.00%> (-5.00%) ⬇️
boa/src/builtins/error/range.rs 65.00% <0.00%> (-5.00%) ⬇️
boa/src/builtins/error/syntax.rs 65.00% <0.00%> (-5.00%) ⬇️
boa/src/builtins/error/reference.rs 65.00% <0.00%> (-5.00%) ⬇️
boa/src/value/conversions.rs 59.01% <0.00%> (-3.28%) ⬇️
... and 54 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 a2e5dca...6af409e. Read the comment docs.

@Razican Razican added the vm Issues and PRs related to the Boa Virtual Machine. label Jan 6, 2021
@jasonwilliams
Copy link
Member

@stephanemagnenat sorry for the conflict, we have a few PRs in this area at the moment

@stephanemagnenat
Copy link
Contributor Author

Do you want me to fix the conflict (by using an optional result), or is there a lot ongoing and I should rather wait that the new code is completed?

@RageKnify RageKnify added this to the v0.12.0 milestone Feb 9, 2021
@RageKnify RageKnify added the execution Issues or PRs related to code execution label Feb 9, 2021
@jasonwilliams
Copy link
Member

@stephanemagnenat you’re free to carry on now it all went in

@stephanemagnenat
Copy link
Contributor Author

stephanemagnenat commented Feb 17, 2021

I've rebased my changes on master.

@stephanemagnenat
Copy link
Contributor Author

I was not fully sure for the naming of the macro, between binop (currently), and bin_op, the current one making vscode's spellchecking complain.

@RageKnify
Copy link
Member

I was not fully sure for the naming of the macro, between binop (currently), and bin_op, the current one making vscode's spellchecking complain.

I agree with code, bin_op makes it easier to read as two separate words, I'd also suggest changing the name of the macro's argument from $e to $op as in operation.

@stephanemagnenat
Copy link
Contributor Author

Done!

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

Looks good!

@jasonwilliams
Copy link
Member

Thanks for the contribution @stephanemagnenat !

@jasonwilliams
Copy link
Member

I do not know if the vm feature is enabled in CI.

No it isn't currently but we hope to change that some point in the future

@jasonwilliams jasonwilliams merged commit f3f3949 into boa-dev:master Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execution Issues or PRs related to code execution vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VM executes ge on value for gt instruction
4 participants