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

try_catch #543

Merged
merged 14 commits into from
Jul 15, 2020
Merged

try_catch #543

merged 14 commits into from
Jul 15, 2020

Conversation

dherman
Copy link
Collaborator

@dherman dherman commented Jul 7, 2020

This PR implements the cx.try_catch API of RFC 29. I had to make a few changes to the signature of try_catch to make the types work out; I'll update the RFC to match.

Still left to do:

  • hide the public API (and tests) behind a feature flag
  • add tests for nested TryCatch instances on the stack
  • add panic handler and tests to ensure panics get caught by try_catch
  • use MaybeUninit instead of mem::zeroed()
  • document the use of Box and the "unlikely" branch
  • fix the N-API tests
  • implement N-API backend
  • panic instead of returning undefined on unexpected Err(Throw)
  • fix merge conflicts
  • return T: Value instead of JsValue

…till left to do:

- hide the public API (and tests) behind a feature flag
- add tests for nested `TryCatch` instances on the stack
- add panic handler and tests to ensure panics get caught by try_catch
src/context/internal.rs Outdated Show resolved Hide resolved
src/context/internal.rs Outdated Show resolved Hide resolved
src/context/internal.rs Outdated Show resolved Hide resolved
crates/neon-sys/native/src/neon.cc Outdated Show resolved Hide resolved
src/context/internal.rs Outdated Show resolved Hide resolved
@dherman dherman marked this pull request as ready for review July 11, 2020 17:25
crates/neon-runtime/src/napi/error.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/error.rs Show resolved Hide resolved
src/context/internal.rs Show resolved Hide resolved
src/context/internal.rs Outdated Show resolved Hide resolved
src/context/internal.rs Outdated Show resolved Hide resolved
src/context/mod.rs Outdated Show resolved Hide resolved
src/context/mod.rs Outdated Show resolved Hide resolved
@@ -226,6 +226,22 @@ pub trait Context<'a>: ContextInternal<'a> {
result
}

#[cfg(all(feature = "try-catch-api", feature = "napi-runtime"))]
Copy link
Member

Choose a reason for hiding this comment

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

Note: For the final documented version, it would be beneficial to include the behavior that this method will panic if a user manually constructs an Err(Throw).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, when we un-feature-gate it, it'll need API docs. In the meantime I'll make sure that gets added to the RFC.

Copy link
Member

@amilajack amilajack Jul 15, 2020

Choose a reason for hiding this comment

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

I'll try to add an Exceptions section to the guides docs soon

@dherman dherman merged commit 7e1ca41 into master Jul 15, 2020
@dherman dherman deleted the try-catch-internal branch July 15, 2020 00:19
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.

3 participants