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

rust: use specific error for traps in execute #735

Merged
merged 4 commits into from
May 27, 2022
Merged

rust: use specific error for traps in execute #735

merged 4 commits into from
May 27, 2022

Conversation

axic
Copy link
Member

@axic axic commented Feb 12, 2021

No description provided.

bindings/rust/src/lib.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #735 (915049c) into master (12505e9) will decrease coverage by 0.01%.
The diff coverage is 95.00%.

❗ Current head 915049c differs from pull request most recent head a38f1e3. Consider uploading reports for the commit a38f1e3 to get more accurate results

@@            Coverage Diff             @@
##           master     #735      +/-   ##
==========================================
- Coverage   99.28%   99.27%   -0.02%     
==========================================
  Files          88       88              
  Lines       13248    13154      -94     
==========================================
- Hits        13153    13058      -95     
- Misses         95       96       +1     
Flag Coverage Δ
rust 98.47% <95.00%> (-0.23%) ⬇️
spectests 89.98% <ø> (ø)
unittests 99.21% <ø> (ø)
unittests-32 99.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bindings/rust/src/lib.rs 98.76% <95.00%> (-0.21%) ⬇️

Copy link
Collaborator

@chfast chfast left a comment

Choose a reason for hiding this comment

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

There are no examples using this, so can't tell if it is useful or not. Can't comment if this is rusty enough.

@axic
Copy link
Member Author

axic commented Feb 15, 2021

There are no examples using this, so can't tell if it is useful or not. Can't comment if this is rusty enough.

I thought I pushed the test changes. Anyway, it removes one level of indirection.

let ret = instance.execute("test", [])?; // this would bubble up errors
if let Some(ret) = ret {
  println!("Returned {}", ret.as_u32());
}

let ret = instance.execute("test", []);
if ret.err() == Error::Trap {
  println!("It's a trap!");
}

@axic axic force-pushed the rust-error-trap branch 3 times, most recently from b2a0986 to 65c0da2 Compare May 24, 2022 14:46
@axic axic marked this pull request as ready for review May 24, 2022 15:13
@axic axic requested review from gumb0 and chfast May 24, 2022 15:13
@axic
Copy link
Member Author

axic commented May 24, 2022

Not sure about the last commit, but otherwise this feels more native to Rust.

@axic axic force-pushed the rust-error-trap branch from d8cd9a4 to 845d90e Compare May 24, 2022 23:37
@axic axic force-pushed the rust-error-trap branch 2 times, most recently from 1835ec4 to bf5695d Compare May 25, 2022 09:24
@axic axic changed the base branch from master to rust-test May 25, 2022 09:24
@axic axic force-pushed the rust-error-trap branch 6 times, most recently from 22adb38 to 0f82440 Compare May 25, 2022 09:56
sys::FizzyValueTypeI64 => TypedValue::U64(unsafe { self.result.value.i64 }),
sys::FizzyValueTypeF32 => TypedValue::F32(unsafe { self.result.value.f32 }),
sys::FizzyValueTypeF64 => TypedValue::F64(unsafe { self.result.value.f64 }),
pub fn typed_value(&self, expected_type: sys::FizzyValueType) -> Option<TypedValue> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if should have this helper or not, e.g. to keep the last commit or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it, but see comment below.

Base automatically changed from rust-test to master May 25, 2022 10:33
@axic axic force-pushed the rust-error-trap branch from 0f82440 to 834cde8 Compare May 25, 2022 10:33

pub fn typed_value(&self, expected_type: sys::FizzyValueType) -> Option<TypedValue> {
assert!(!self.0.trapped);
if self.0.has_value {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now the decision what to return is based on both expected_type and self.0.has_value

Maybe better change this to if expected_type != sys::FizzyValueTypeVoid and assert inside assert!(self.0.has_value). Or even make it part of one bigger match expected_type.

Also maybe rename expected_type to just type, otherwise it looks like it's mainly not a conversion, but a check / test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a commit flipping conditions, but it doesn't make much of a difference. assert!'s are always in the binary, debug_assert!s aren't.

type is a keyword, hence expected_type.

@axic axic force-pushed the rust-error-trap branch from 915049c to a38f1e3 Compare May 27, 2022 15:29
@axic axic merged commit 556a526 into master May 27, 2022
@axic axic deleted the rust-error-trap branch May 27, 2022 15:54
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