Skip to content

try_catch#543

Merged
dherman merged 14 commits intomasterfrom
try-catch-internal
Jul 15, 2020
Merged

try_catch#543
dherman merged 14 commits intomasterfrom
try-catch-internal

Conversation

@dherman
Copy link
Copy Markdown
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
Comment thread src/context/internal.rs Outdated
Comment thread src/context/internal.rs Outdated
Comment thread src/context/internal.rs Outdated
Comment thread crates/neon-sys/native/src/neon.cc Outdated
Comment thread src/context/internal.rs Outdated
Comment thread src/context/internal.rs
@dherman dherman marked this pull request as ready for review July 11, 2020 17:25
Comment thread crates/neon-runtime/src/napi/error.rs Outdated
Comment thread crates/neon-runtime/src/napi/error.rs
Comment thread src/context/internal.rs
Comment thread src/context/internal.rs Outdated
Comment thread src/context/internal.rs Outdated
Comment thread src/context/mod.rs Outdated
Comment thread src/context/mod.rs Outdated
Comment thread src/context/mod.rs
result
}

#[cfg(all(feature = "try-catch-api", feature = "napi-runtime"))]
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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