wasmtime / PR #1243 [wasi-common]: winx now returns io::E... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1243 [wasi-common]: winx now returns io::E...

Wasmtime GitHub notifications bot (Mar 06 2020 at 13:55):

kubkon opened PR #1243 from winx-io-error to master:

This commit is a spiritual follower of #1242 in the sense that it adjusts winx to also return io::Error directly rather than tossing a custom error type here and there.

While here, I've also done a little bit of refactoring as suggested by @alexcrichton in #1226: whenever we re-map error values, just use plain old match (or if let), and do early returns.

Wasmtime GitHub notifications bot (Mar 06 2020 at 13:55):

kubkon requested peterhuene, marmistrz, and sunfishcode for a review on PR #1243.

Wasmtime GitHub notifications bot (Mar 06 2020 at 13:55):

kubkon requested peterhuene, marmistrz, and sunfishcode for a review on PR #1243.

Wasmtime GitHub notifications bot (Mar 06 2020 at 13:55):

kubkon requested peterhuene, marmistrz, and sunfishcode for a review on PR #1243.

Wasmtime GitHub notifications bot (Mar 06 2020 at 13:57):

kubkon updated PR #1243 from winx-io-error to master:

This commit is a spiritual follower of #1242 in the sense that it adjusts winx to also return io::Error directly rather than tossing a custom error type here and there.

While here, I've also done a little bit of refactoring as suggested by @alexcrichton in #1226: whenever we re-map error values, just use plain old match (or if let), and do early returns.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:01):

kubkon edited PR #1243 from winx-io-error to master:

This commit is a spiritual descendant of #1242 in the sense that it adjusts winx to also return io::Error directly rather than tossing a custom error type here and there.

While here, I've also done a little bit of refactoring as suggested by @alexcrichton in #1226: whenever we re-map error values, just use plain old match (or if let), and do early returns.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:40):

kubkon edited PR #1243 from winx-io-error to master:

This commit is a spiritual successor of #1242 in the sense that it adjusts winx to also return io::Error directly rather than tossing a custom error type here and there.

While here, I've also done a little bit of refactoring as suggested by @alexcrichton in #1226: whenever we re-map error values, just use plain old match (or if let), and do early returns.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:10):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:10):

alexcrichton created PR Review Comment:

Or even better yet:

fn extract_raw_os_error(e: io::Error) -> Result<i32> {
    // match returning EIO for "other errors"
}

let err = match fs::rename(...) {
    Ok(()) => return Ok(()),
    Err(e) => extract_raw_os_error(e)?,
};
// de-indented error-handling code
~~~

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:10):

alexcrichton created PR Review Comment:

FWIW this'd probably be a good place to do something like:

let err = match fs::rename(...) {
    Ok(()) => return Ok(()),
    Err(e) => e,
};
// de-indented error-handling code
~~~

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:10):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:10):

alexcrichton created PR Review Comment:

One thing I've found helpful personally is to consistently use the same error handling pattern, e.g. using match everywhere or using map_err everywhere. (I realize this is existing code so don't put too much weight on this, just wanted to leave a passing comment about this)

Basically I think it'd be better to use the same idioms here (not map_err) to handle errors as elsewhere.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:30):

kubkon created PR Review Comment:

Nice, I like it! I'll try and enforce this principle whenever I write similar code in the future. Also, while I won't do this in this PR, when I'm less creative I might scan wasi-common and related, and rewrite any suitable occurrence using this principle. :+1:

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:30):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:31):

kubkon created PR Review Comment:

Oh oops, the reply to the above should have been written down here actually.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:31):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:31):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:50):

kubkon updated PR #1243 from winx-io-error to master:

This commit is a spiritual successor of #1242 in the sense that it adjusts winx to also return io::Error directly rather than tossing a custom error type here and there.

While here, I've also done a little bit of refactoring as suggested by @alexcrichton in #1226: whenever we re-map error values, just use plain old match (or if let), and do early returns.

Wasmtime GitHub notifications bot (Mar 06 2020 at 21:15):

kubkon updated PR #1243 from winx-io-error to master:

This commit is a spiritual successor of #1242 in the sense that it adjusts winx to also return io::Error directly rather than tossing a custom error type here and there.

While here, I've also done a little bit of refactoring as suggested by @alexcrichton in #1226: whenever we re-map error values, just use plain old match (or if let), and do early returns.

Wasmtime GitHub notifications bot (Mar 06 2020 at 21:24):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 07 2020 at 08:30):

kubkon updated PR #1243 from winx-io-error to master:

This commit is a spiritual successor of #1242 in the sense that it adjusts winx to also return io::Error directly rather than tossing a custom error type here and there.

While here, I've also done a little bit of refactoring as suggested by @alexcrichton in #1226: whenever we re-map error values, just use plain old match (or if let), and do early returns.

Wasmtime GitHub notifications bot (Mar 07 2020 at 12:33):

marmistrz submitted PR Review.

Wasmtime GitHub notifications bot (Mar 07 2020 at 12:33):

marmistrz submitted PR Review.

Wasmtime GitHub notifications bot (Mar 07 2020 at 12:33):

marmistrz created PR Review Comment:

I'd say that or_else is more readable

Wasmtime GitHub notifications bot (Mar 07 2020 at 12:33):

marmistrz created PR Review Comment:

Will this conversion always succeed?

Wasmtime GitHub notifications bot (Mar 07 2020 at 12:33):

marmistrz created PR Review Comment:

Are you sure that you want to modify the snapshot?

Wasmtime GitHub notifications bot (Mar 07 2020 at 12:33):

marmistrz created PR Review Comment:

I'd say that without returns the control flow is easier to follow.

Wasmtime GitHub notifications bot (Mar 07 2020 at 12:33):

marmistrz created PR Review Comment:

How about adding a comment why unwrap will always succeed (possibly using expect)?

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:45):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:45):

kubkon created PR Review Comment:

AFAIK, on Windows, io::Error populated with raw OS error (as is the case here) will never store a value that's negative since GetLastError() always returns a DWORD aka u32. So the only case when this case would silently fail would be if code value had overflown u32 which is impossible given that io::Error::from_raw_os_error(code: i32) expects an i32 as its argument. Also, it's worth noting that this approach matches that of Rust's libstd, so I think we should be safe here.

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:47):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:47):

kubkon created PR Review Comment:

Hmm, I actually have to agree with @alexcrichton on this one, and prefer explicit match/if let with early returns.

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:47):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:47):

kubkon created PR Review Comment:

See above :-)

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:48):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:48):

kubkon created PR Review Comment:

The changes to the snapshot do not touch either its use of the WASI ABI nor the functional aspects of the code which are identical between both snapshots anyhow, so we're fine here.

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:50):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 07 2020 at 14:50):

kubkon created PR Review Comment:

See discussion in #1242, plus raw_os_error docs.

Wasmtime GitHub notifications bot (Mar 08 2020 at 11:02):

marmistrz submitted PR Review.

Wasmtime GitHub notifications bot (Mar 09 2020 at 09:32):

kubkon merged PR #1243.

Last updated: Nov 22 2024 at 16:03 UTC