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

Add Function::{ScriptId, GetScriptOrigin}, ScriptOrigin::Get{ScriptId, ResourceName, SourceMapUrl} bindings #1250

Merged
merged 6 commits into from
Jun 28, 2023

Conversation

rakeeb-hossain
Copy link
Contributor

I added the bindings in the title to use in an application! Let me know if I'm missing anything.

@CLAassistant
Copy link

CLAassistant commented Jun 21, 2023

CLA assistant check
All committers have signed the CLA.

src/function.rs Outdated

/// Returns scriptId.
#[inline(always)]
pub fn get_script_id(&self) -> Option<u32> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be i32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was unsure about this. V8 docs are sparse on when a script ID is < 0, but the only cases I could find in the source were 1) uninitialized ScriptOrigin classes (value is -1) and 2) kTemporaryScriptId = -2. Due to uncertainty on when these cases actually occur, I just treated these as None values.

I can change this to i32 instead if you think that would make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmastrac Would you suggest i32 instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should use i32 here. Also please remove get_ prefix - in v8/include/v8-function.h this function is named ScriptId()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Almost there, just a few nitpicks to match V8 API

src/script.rs Outdated Show resolved Hide resolved
src/script.rs Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@bartlomieju bartlomieju merged commit 7419b38 into denoland:main Jun 28, 2023
7 checks passed
@rakeeb-hossain
Copy link
Contributor Author

rakeeb-hossain commented Jun 28, 2023

@bartlomieju Great! When do you expect deno_core to cut a release that supports the current head of rusty_v8? I noticed that the head of rusty_v8 is not compatible with the current head of deno_core due to a change in the CFunctionInfo::new API.

I currently depend on these for my project!

@bartlomieju
Copy link
Member

@rakeeb-hossain we will release next week. Waiting on one V8 upgrade that fixes a long standing bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants