support non-result return values in jsg rust #5743
Conversation
anonrig
force-pushed
the
yagiz/support-non-result-return-vals
branch
from
fa13c8a to
33688a1
Compare
|
The generated output of |
anonrig
force-pushed
the
yagiz/support-non-result-return-vals
branch
from
f1c9bcb to
95722ed
Compare
CodSpeed Performance ReportMerging #5743 will improve performance by 12.69%Comparing Summary
Benchmarks breakdown
Footnotes
|
| use crate::v8; | ||
| use crate::v8::ToLocalValue; | ||
|
|
||
| impl Type for String { |
There was a problem hiding this comment.
nit: let's move these impl Type below Wrappable definition
| fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value>; | ||
|
|
||
| /// Converts a JavaScript value into this Rust type. | ||
| fn unwrap(isolate: v8::IsolatePtr, value: v8::Local<v8::Value>) -> Self; |
There was a problem hiding this comment.
imo this should always return some Result, I'm not sure how will error handling work otherwise
| /// Converts a JavaScript value into this Rust type. | ||
| fn unwrap(isolate: v8::IsolatePtr, value: v8::Local<v8::Value>) -> Self; | ||
|
|
||
| /// Attempts to unwrap a JavaScript value, returning None and throwing a JS error on failure. |
There was a problem hiding this comment.
what does throwing a JS error mean here in Rust context? panic? we shouldn't write code that is expected to panic.
| fn wrap(self, lock: &mut Lock) -> v8::Local<'_, v8::Value> { | ||
| match self { | ||
| Ok(value) => value.wrap(lock), | ||
| Err(_) => unreachable!("Cannot wrap Result::Err"), |
There was a problem hiding this comment.
why is it unreachable? this will also panic
| } | ||
|
|
||
| fn unwrap(_isolate: v8::IsolatePtr, _value: v8::Local<v8::Value>) -> Self { | ||
| unreachable!("Cannot unwrap into Result") |
There was a problem hiding this comment.
here too: if this is unreachable then unwrap should not be part of Wrappable but be in Unwrappable. Otherwise someone will crash the code.
mikea
left a comment
mikea
left a comment
There was a problem hiding this comment.
I think error handling design needs a bit more love.
2 participants
No description provided.