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

Implementation of instanceof operator #908

Merged
merged 1 commit into from
Oct 28, 2020
Merged

Implementation of instanceof operator #908

merged 1 commit into from
Oct 28, 2020

Conversation

morrien
Copy link
Contributor

@morrien morrien commented Oct 22, 2020

  • fixed @@hasinstance symbol call
  • added get_method() and ordinary_has_instance() for GcObject
  • implemented instanceof operator

This Pull Request fixes/closes #796.

Copy link
Member

@HalidOdat HalidOdat 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! this PR needs a rebase, and we should also add some tests :)

boa/src/object/gcobject.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/node/operator/bin_op/mod.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat added enhancement New feature or request execution Issues or PRs related to code execution labels Oct 22, 2020
@HalidOdat HalidOdat added this to the v0.11.0 milestone Oct 22, 2020
@codecov
Copy link

codecov bot commented Oct 26, 2020

Codecov Report

Merging #908 into master will increase coverage by 0.01%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #908      +/-   ##
==========================================
+ Coverage   59.18%   59.19%   +0.01%     
==========================================
  Files         166      166              
  Lines       10515    10545      +30     
==========================================
+ Hits         6223     6242      +19     
- Misses       4292     4303      +11     
Impacted Files Coverage Δ
boa/src/object/mod.rs 46.45% <ø> (ø)
boa/src/object/gcobject.rs 64.20% <47.61%> (-1.40%) ⬇️
boa/src/syntax/ast/node/operator/bin_op/mod.rs 73.50% <63.63%> (+2.20%) ⬆️
boa/src/builtins/symbol/mod.rs 77.27% <100.00%> (ø)

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 fd181e2...8a9c728. Read the comment docs.

@morrien
Copy link
Contributor Author

morrien commented Oct 26, 2020

  • fixed throw formatting
  • added tests for instanceof operator and ordinary_has_instance()
  • rebased the PR

@HalidOdat could you please explain how to resolve the remaining failing checks? Thank you.

@Razican
Copy link
Member

Razican commented Oct 27, 2020

* fixed `throw` formatting

* added tests for `instanceof` operator and `ordinary_has_instance()`

* rebased the PR

@HalidOdat could you please explain how to resolve the remaining failing checks? Thank you.

It seems that this requires running cargo fmt. I will review the code, since it also seems to require a small change to fix clippy lints. The test262 issue should be solved in #914.

boa/src/object/gcobject.rs Outdated Show resolved Hide resolved
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.

I added some suggestions on new lines at the end of files. This should be added by any editor that takes the editorconfig into account, but it seems it didn't happen in this case.

boa/src/object/tests.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/node/operator/tests.rs Outdated Show resolved Hide resolved
@morrien
Copy link
Contributor Author

morrien commented Oct 27, 2020

I ran cargo fmt, which also added the EOF newlines, and I fixed the clippy warning sa well.
I'll wait for #914 to be merged and rebase the PR again.

The only failed check that remains is code coverage, how could I address that? Thank you.

@Razican
Copy link
Member

Razican commented Oct 28, 2020

I ran cargo fmt, which also added the EOF newlines, and I fixed the clippy warning sa well.
I'll wait for #914 to be merged and rebase the PR again.

Thanks! That one is ready now :)

The only failed check that remains is code coverage, how could I address that? Thank you.

We are currently debating about "patch" coverage failures. We will merge it anyways if that's the only issue.

@morrien
Copy link
Contributor Author

morrien commented Oct 28, 2020

Latest rebase seems to have fixed everything, even somehow improved the code coverage - I believe this is ready for merge.

With that being said, I had to rebase twice, because running cargo clippy would not detect another clippy warning that I noticed only in the "Files changed" tab afterwards - does anyone know what could be causing this? I believe this caused me to miss the first clippy warning regarding return Ok(false) as well, as I only noticed it after the PR went online.

@Razican
Copy link
Member

Razican commented Oct 28, 2020

With that being said, I had to rebase twice, because running cargo clippy would not detect another clippy warning that I noticed only in the "Files changed" tab afterwards - does anyone know what could be causing this? I believe this caused me to miss the first clippy warning regarding return Ok(false) as well, as I only noticed it after the PR went online.

This might be related to rust-lang/rust-clippy#4612

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 good to me, thanks!

Test262 conformance changes:

Test result master count PR count difference
Total 78,415 78,415 0
Passed 18,461 18,945 +484
Ignored 15,547 15,547 0
Failed 44,407 43,923 -484
Panics 1,129 1,127 -2
Conformance 23.54 24.16 +0.62%

@HalidOdat HalidOdat merged commit f1d676e into boa-dev:master Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the instanceof operator
4 participants