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

Rewrite initialization of builtins to use the BuiltIn trait #1586

Merged
merged 2 commits into from
Oct 2, 2021

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Sep 20, 2021

We currently aren't using the BuiltIn trait to check the initialization of builtins, meaning any struct that implements a init function that returns a (&str, Value, Attribute) tuple can be used to initialize a global builtin.

This PR changes the following:

  • Refactors BuiltIn::init to return only a JsValue and converts BuiltIn::attribute into a constant.
  • Refactors the initialization of all builtins to check for an implementation of the BuiltIn trait.
  • Creates a globals! macro to avoid the boilerplate.
  • Refactors all builtins to conform to the new trait.

@jedel1043 jedel1043 added builtins PRs and Issues related to builtins/intrinsics API labels Sep 20, 2021
@github-actions
Copy link

github-actions bot commented Sep 20, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 80,930 80,930 0
Passed 33,192 33,192 0
Ignored 15,898 15,898 0
Failed 31,840 31,840 0
Panics 0 0 0
Conformance 41.01% 41.01% 0.00%

Copy link
Member

@raskad raskad 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 idea to me.

Not sure if we need this macro, as I personally think macros always negatively impact readability and probably(?) also compile times.

@jedel1043
Copy link
Member Author

Not sure if we need this macro, as I personally think macros always negatively impact readability and probably(?) also compile times.

I think this is a good case of use for macros. The alternative would be to manually write 26 calls to init_builtin in succession, which a macro is perfect for, and it clearly defines what are the global constructors of our context, making the implementation of new builtins as easy as putting them in the globals! list and solving any compile errors.

boa/src/builtins/mod.rs Show resolved Hide resolved
@raskad raskad merged commit 1cbbb0f into master Oct 2, 2021
@jedel1043 jedel1043 deleted the builtin_init branch October 2, 2021 21:13
@raskad raskad added this to the v0.14.0 milestone Oct 4, 2021
@RageKnify RageKnify added the Internal Category for changelog label Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API builtins PRs and Issues related to builtins/intrinsics Internal Category for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants