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

fix: remove use of deprecated apis #1488

Merged
merged 1 commit into from
Jun 27, 2024
Merged

fix: remove use of deprecated apis #1488

merged 1 commit into from
Jun 27, 2024

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented May 22, 2024

Fixes: #1478

https://docs.google.com/document/d/1FWnrKxjl0P-jY0CMPlzbXkH8xkKoX6nM4e2OXq1EGHU

tldr some* v8 objects now directly store a "wrapped pointer" field which is traced by cppgc. cppgc no longer requires embedder field information when being set up, and this field is set using Object::wrap/Object::unwrap.

* objects must use the JSObjectWithEmbedderSlots map

@CLAassistant
Copy link

CLAassistant commented May 22, 2024

CLA assistant check
All committers have signed the CLA.

examples/cppgc-object.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

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

LGTM -- will need some small tweaks to deno_core to land this there, I think.

@0f-0b
Copy link

0f-0b commented May 22, 2024

the only way to get one of those afaik is by creating a template with internal fields, even if those internal fields end up not being used.

Another way is by calling a constructor created from a FunctionTemplate. In this case there's no need to add internal fields, which saves 8 bytes per object compared to what is done now.

@devsnek
Copy link
Member Author

devsnek commented May 23, 2024

@0f-0b thanks for the pointer

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM

@devsnek devsnek force-pushed the x/new-cppgc-embedding branch 2 times, most recently from 5836af3 to df8f589 Compare June 20, 2024 20:16
@devsnek devsnek changed the title fix: remove deprecated cppgc embedder api and use new wrap api fix: remove use of deprecated apis Jun 20, 2024
src/object.rs Outdated Show resolved Hide resolved
@devsnek devsnek force-pushed the x/new-cppgc-embedding branch 11 times, most recently from 3081a5f to 2a65ecb Compare June 27, 2024 03:16
Comment on lines +60 to +65
fn empty(
_scope: &mut v8::HandleScope,
_args: v8::FunctionCallbackArguments,
_rv: v8::ReturnValue,
) {
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe increment a counter here to ensure it's called the correct number of times?

Comment on lines -34 to -35
#pragma clang diagnostic ignored "-Wdeprecated-declarations"

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@littledivy littledivy 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!

@devsnek devsnek merged commit e747f40 into main Jun 27, 2024
16 checks passed
@devsnek devsnek deleted the x/new-cppgc-embedding branch June 27, 2024 15:29
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.

Use of deprecated APIs in binding.cc
6 participants