[Merged by Bors] - Redesign native functions and closures API #2499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jedel1043
added
execution
jedel1043
requested review from
Razican,
jasonwilliams,
HalidOdat,
RageKnify,
raskad and
nekevss
Test262 conformance changes
|
Codecov Report
@@ Coverage Diff @@
## main #2499 +/- ##
==========================================
+ Coverage 51.50% 51.64% +0.14%
==========================================
Files 358 356 -2
Lines 35755 35766 +11
==========================================
+ Hits 18414 18473 +59
+ Misses 17341 17293 -48
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Benchmark for 7e18e7aClick to view benchmark
|
|
Will disable the benchmark, since everything looks good. |
jedel1043
force-pushed
the
new-functions
branch
2 times, most recently
from
609a24f to
1d18d8f
Compare
nekevss
reviewed
Dec 28, 2022
There was a problem hiding this comment.
A lot of this looks good to me. NativeCallable makes sense the more I think about it. Maybe, NativeCallableFn to be a bit more specific. I'm not entirely sure about it being in the function module, and after looking at this, I'm also a bit curious about FunctionBuilder too, as we use the term function in a lot of different contexts that it may be ambiguous at times. Maybe function -> native_function/native_fn and FunctionBuilder -> FunctionObjectBuilder.
jedel1043
force-pushed
the
new-functions
branch
from
b08d19a to
6bff1d0
Compare
Sounds good! Though, if we're changing |
jedel1043
force-pushed
the
new-functions
branch
from
70d45d1 to
24d7e04
Compare
raskad
approved these changes
Jan 8, 2023
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 8, 2023
This PR is a complete redesign of our current native functions and closures API. I was a bit dissatisfied with our previous design (even though I created it 😆), because it had a lot of superfluous traits, a forced usage of `Gc<GcCell<T>>` and an overly restrictive `NativeObject` bound. This redesign, on the other hand, simplifies a lot our public API, with a simple `NativeCallable` struct that has several constructors for each type of required native function. This new design doesn't require wrapping every capture type with `Gc<GcCell<T>>`, relaxes the trait requirement to `Trace + 'static` for captures, can be reused in both `JsObject` functions and (soonish) host defined functions, and is (in my opinion) a bit cleaner than the previous iteration. It also offers an `unsafe` API as an escape hatch for users that want to pass non-Copy closures which don't capture traceable types. Would ask for bikeshedding about the names though, because I don't know if `NativeCallable` is the most precise name for this. Same about the constructor names; I added the `from` prefix to all of them because it's the "standard" practice, but seeing the API doesn't have any other method aside from `call`, it may be better to just remove the prefix altogether. Let me know what you think :)
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a complete redesign of our current native functions and closures API.
I was a bit dissatisfied with our previous design (even though I created it 😆), because it had a lot of superfluous traits, a forced usage of
Gc<GcCell<T>>and an overly restrictiveNativeObjectbound. This redesign, on the other hand, simplifies a lot our public API, with a simpleNativeCallablestruct that has several constructors for each type of required native function.This new design doesn't require wrapping every capture type with
Gc<GcCell<T>>, relaxes the trait requirement toTrace + 'staticfor captures, can be reused in bothJsObjectfunctions and (soonish) host defined functions, and is (in my opinion) a bit cleaner than the previous iteration. It also offers anunsafeAPI as an escape hatch for users that want to pass non-Copy closures which don't capture traceable types.Would ask for bikeshedding about the names though, because I don't know if
NativeCallableis the most precise name for this. Same about the constructor names; I added thefromprefix to all of them because it's the "standard" practice, but seeing the API doesn't have any other method aside fromcall, it may be better to just remove the prefix altogether.Let me know what you think :)