-
Notifications
You must be signed in to change notification settings - Fork 347
Expose ModifyCodeGenerationFromStrings
callback
#1726
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
base: main
Are you sure you want to change the base?
Conversation
6b5758d
to
158437c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the patch - test could use some improvement
CALLED.store(true, Ordering::Relaxed); | ||
v8::ModifyCodeGenerationFromStringsResult { | ||
codegen_allowed: CODEGEN_ALLOWED.load(Ordering::Relaxed), | ||
modified_source: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use modified_source
to demonstrate the feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for the late reply!
All relevant APIs used to interact with V8 strings seem to require a reference to a HandleScope
, which isn't available in the callback. Therefore, it doesn't seem to be possible to create a new string for the modified source. I could return the source
parameter unmodified, but the effect would be equivalent to returning None
anyway.
I'm aware that this is a pretty big limitation. I guess providing a HandleScope
in the callback would be more complex than the current solution where I'm directly passing the extern "C"
function, but I could try to look into how some of the other callbacks are implemented.
static CODEGEN_ALLOWED: AtomicBool = AtomicBool::new(false); | ||
static CALLED: AtomicBool = AtomicBool::new(false); | ||
|
||
#[allow(improper_ctypes_definitions)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should change how Local
is defined to avoid this..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure how to fix this either. It pretty much boils down to a Option<NonNull<T>>
, which should be safe over FFI, but maybe the type with PhantomData
is too complex for the compiler to detect it as such.
158437c
to
20560e0
Compare
Implements #1716. Most of the code was taken from #435 and adapted to work with the current V8 version (credits for the original PR go to @NeoLegends).
The approach implemented in this PR is somewhat limited (but does match the feature set of the previous PR):
HandleScope
available in the callback, so it doesn't seem to be possible to read or modify the source code#[allow(improper_ctypes_definitions)]
must be added to the callback function for it to not be (as far as I know, incorrectly) flagged with a warning