Stream: git-wasmtime

Topic: wasmtime / PR #1255 [wasi-common]: move filetime module t...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 17:59):

kubkon opened PR #1255 from filetime-in-yanix to master:

I've noticed that we could replace every occurrence of crate::Result
in filetime mods with io::Result, so I thought why not move it
to yanix and get rid off a lot of unnecessary code duplication
within wasi-common. Now, ideally I'd have our filetime modifications
backported to @alexcrichton's [filetime] crate, but one step at a time
(apologies @alexcrichton, I was meant to backport this ages ago, just didn't
find the time yet... :-().

Anyway, this commit does just that; i.e., moves the filetime modules
into yanix which seems a better fit for this type of code.

[filetime]: https://github.com/alexcrichton/filetime

There is one caveat here. On Emscripten, converting between filetime::Filetime
and libc::timespec appears to be lossy, at least as far as the
types are concerned. Now, filetime::Filetime's seconds field is
i64 while nanoseconds field is u32, while Emscripten's
libc::timespec requires both to be i32 width. This might actually
not be a problem since I don't think it's possible to fill filetime::Filetime
struct with values of width wider than i32 since Emscripten is 32bit
but just to be on the safe side, we do a TryInto conversion, log
the error (if any), and return libc::EOVERFLOW.

Lemme know what you guys reckon!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 17:59):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1255.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 17:59):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1255.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 18:00):

kubkon updated PR #1255 from filetime-in-yanix to master:

I've noticed that we could replace every occurrence of crate::Result
in filetime mods with io::Result, so I thought why not move it
to yanix and get rid off a lot of unnecessary code duplication
within wasi-common. Now, ideally I'd have our filetime modifications
backported to @alexcrichton's [filetime] crate, but one step at a time
(apologies @alexcrichton, I was meant to backport this ages ago, just didn't
find the time yet... :-().

Anyway, this commit does just that; i.e., moves the filetime modules
into yanix which seems a better fit for this type of code.

[filetime]: https://github.com/alexcrichton/filetime

There is one caveat here. On Emscripten, converting between filetime::Filetime
and libc::timespec appears to be lossy, at least as far as the
types are concerned. Now, filetime::Filetime's seconds field is
i64 while nanoseconds field is u32, while Emscripten's
libc::timespec requires both to be i32 width. This might actually
not be a problem since I don't think it's possible to fill filetime::Filetime
struct with values of width wider than i32 since Emscripten is 32bit
but just to be on the safe side, we do a TryInto conversion, log
the error (if any), and return libc::EOVERFLOW.

Lemme know what you guys reckon!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 20:38):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 20:38):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 20:38):

sunfishcode created PR Review Comment:

Slightly more verbose, but avoids the need for a two-line comment: i64::from(ft.nanoseconds()) :-).

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:45):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:45):

kubkon created PR Review Comment:

Excellent, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:51):

kubkon updated PR #1255 from filetime-in-yanix to master:

I've noticed that we could replace every occurrence of crate::Result
in filetime mods with io::Result, so I thought why not move it
to yanix and get rid off a lot of unnecessary code duplication
within wasi-common. Now, ideally I'd have our filetime modifications
backported to @alexcrichton's [filetime] crate, but one step at a time
(apologies @alexcrichton, I was meant to backport this ages ago, just didn't
find the time yet... :-().

Anyway, this commit does just that; i.e., moves the filetime modules
into yanix which seems a better fit for this type of code.

[filetime]: https://github.com/alexcrichton/filetime

There is one caveat here. On Emscripten, converting between filetime::Filetime
and libc::timespec appears to be lossy, at least as far as the
types are concerned. Now, filetime::Filetime's seconds field is
i64 while nanoseconds field is u32, while Emscripten's
libc::timespec requires both to be i32 width. This might actually
not be a problem since I don't think it's possible to fill filetime::Filetime
struct with values of width wider than i32 since Emscripten is 32bit
but just to be on the safe side, we do a TryInto conversion, log
the error (if any), and return libc::EOVERFLOW.

Lemme know what you guys reckon!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 09:40):

kubkon updated PR #1255 from filetime-in-yanix to master:

I've noticed that we could replace every occurrence of crate::Result
in filetime mods with io::Result, so I thought why not move it
to yanix and get rid off a lot of unnecessary code duplication
within wasi-common. Now, ideally I'd have our filetime modifications
backported to @alexcrichton's [filetime] crate, but one step at a time
(apologies @alexcrichton, I was meant to backport this ages ago, just didn't
find the time yet... :-().

Anyway, this commit does just that; i.e., moves the filetime modules
into yanix which seems a better fit for this type of code.

[filetime]: https://github.com/alexcrichton/filetime

There is one caveat here. On Emscripten, converting between filetime::Filetime
and libc::timespec appears to be lossy, at least as far as the
types are concerned. Now, filetime::Filetime's seconds field is
i64 while nanoseconds field is u32, while Emscripten's
libc::timespec requires both to be i32 width. This might actually
not be a problem since I don't think it's possible to fill filetime::Filetime
struct with values of width wider than i32 since Emscripten is 32bit
but just to be on the safe side, we do a TryInto conversion, log
the error (if any), and return libc::EOVERFLOW.

Lemme know what you guys reckon!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 15:07):

kubkon merged PR #1255.

Last updated: Oct 23 2024 at 20:03 UTC