Skip to content
This repository was archived by the owner on Jul 12, 2024. It is now read-only.

Fix #76: Implement jl.Class.isInstance for JS classes. #95

Merged
merged 1 commit into from
Apr 13, 2024

Conversation

sjrd
Copy link
Collaborator

@sjrd sjrd commented Apr 11, 2024

No description provided.

@sjrd sjrd requested a review from tanishiking April 11, 2024 16:07
@sjrd sjrd force-pushed the isinstance-for-js-classes branch from e4b5f80 to be00828 Compare April 11, 2024 16:44
@sjrd sjrd linked an issue Apr 11, 2024 that may be closed by this pull request
@sjrd sjrd force-pushed the isinstance-for-js-classes branch from be00828 to fd9f08a Compare April 11, 2024 19:01
@sjrd sjrd force-pushed the isinstance-for-js-classes branch from fd9f08a to 33d23a9 Compare April 12, 2024 08:22
Copy link
Owner

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!
I left one note for myself, if my understanding is wrong, please let me know 😄

instrs += I32_CONST(0)
instrs += RETURN
}
instrs += LOCAL_SET(valueNonNullLocal)
Copy link
Owner

Choose a reason for hiding this comment

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

[note]
In most cases, null check here isn't needed, and moved into the j.l.Object case + case _ =>, and we can reduce instructions in most scenaios

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is true, though the most important thing is that if we need to throw the TypeError, we need to throw it even if the argument is null. So eagerly testing for null was not working.

@tanishiking tanishiking merged commit 7adcb8d into tanishiking:main Apr 13, 2024
1 check passed
@sjrd sjrd deleted the isinstance-for-js-classes branch April 13, 2024 05:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuntimeError: unreachable (in the isInstance helper)
2 participants